Moodle
  1. Moodle
  2. MDL-34057

styles_debug returns 404s on busy sites

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.6
    • Fix Version/s: 2.4
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      1/ enable themedesigner mode
      2/ run the attached jmeter script (modify it to match your site address)
      3/ visit site using normal browser and make sure all styles load
      4/ modify CSS in source code
      5/ verify the change appears in browser

      Show
      1/ enable themedesigner mode 2/ run the attached jmeter script (modify it to match your site address) 3/ visit site using normal browser and make sure all styles load 4/ modify CSS in source code 5/ verify the change appears in browser
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w28_MDL-34057_m24_styledebug
    • Rank:
      42360

      Description

      Was accidentally load testing with theme designer mode turned on, but it highlighted a potential issue.

      I was occasionally getting 404s returned by styles_debug.php and these seem to stem from:

      if (!file_exists($candidatesheet)) {
          css_send_css_not_found();
      }
      

      Looking into the commit that introduced this, I think that it could potentially be a race condition on busy sites.

      The original commit which introduced this was 7829cf7955174a561ea9e1d736085342842cf738 (MDL-21208).

      From the way I read it, the serialized CSS is generated in lib/outputlib.php::css_urls() if the theme revision is -1 (e.g. theme designer mode is on).
      It looks like if the candidate sheet exists but is older than the THEME_DESIGNER_CACHE_LIFETIME (defaults to 4 seconds), then:

      • the existing candidate sheet is ulinked
      • the css content is generated
      • the new candidate sheet is serialized and put in place

      As a result, if multiple requests are hitting a site then there's a potential for:

      • Request to Client A is served
        • css_urls generates the candidatesheet
      • Some content is served to Client A
      • Request to Client B is served 5 seconds later
        • The candidatesheet is stale so unlinked
        • New CSS generation begins
      • Client A starts downloading style sheets
        • canidatesheet does not exist so a styles_debug.php returns a 404
      • Client B's CSS generation completes
        • CSS Content is serialized
        • serialized CSS content written to candidatesheet
      • Client B retrieves content

        Activity

        Hide
        Andrew Nicols added a comment -

        I suspect that this is one for you Petr

        Show
        Andrew Nicols added a comment - I suspect that this is one for you Petr
        Hide
        Andrew Nicols added a comment -

        Easy way to reduce the potential impact:

        Change:

        unlink($candidatesheet);
        $css = $this->css_content($optimiser);
        file_put_contents($candidatesheet, serialize($css));
        

        To:

        $css = serialize($this->css_content($optimiser));
        unlink($candidatesheet);
        file_put_contents($candidatesheet, $css);
        

        Correct and atomic way:

        $candidatesheettemp = // some way of creating a uniquely named file which doesn't already exist
        $css = serialize($this->css_content($optimiser));
        file_put_contents($candidatesheettemp, $css);
        rename($candidatesheettemp, $candidatesheet);
        

        From what I understand of the php rename function, it should be safe to rename without an unlink first. The difficulty here is generating a unique filename...

        Show
        Andrew Nicols added a comment - Easy way to reduce the potential impact: Change: unlink($candidatesheet); $css = $ this ->css_content($optimiser); file_put_contents($candidatesheet, serialize($css)); To: $css = serialize($ this ->css_content($optimiser)); unlink($candidatesheet); file_put_contents($candidatesheet, $css); Correct and atomic way: $candidatesheettemp = // some way of creating a uniquely named file which doesn't already exist $css = serialize($ this ->css_content($optimiser)); file_put_contents($candidatesheettemp, $css); rename($candidatesheettemp, $candidatesheet); From what I understand of the php rename function, it should be safe to rename without an unlink first. The difficulty here is generating a unique filename...
        Hide
        Andrew Nicols added a comment -

        The attached jmeter test script is enough to demonstrate the issue for me (with theme designer on). With your speedy SSDs, you may need to increase the number of threads. You'll also need to correct the URL in the HTTP Request to suit.

        If you view the Results Tree, you'll see that periodically request get errors, and expanding the tree you should be able to find the individual request to styles_debug.php

        Show
        Andrew Nicols added a comment - The attached jmeter test script is enough to demonstrate the issue for me (with theme designer on). With your speedy SSDs, you may need to increase the number of threads. You'll also need to correct the URL in the HTTP Request to suit. If you view the Results Tree, you'll see that periodically request get errors, and expanding the tree you should be able to find the individual request to styles_debug.php
        Hide
        Petr Škoda added a comment -

        Hi, I was aware of this problem when writing the debug mode for themes, the problem here is that it was not intended for any production servers, it was designed only for developer installs with few requests for people writing new themes or debugging CSS problems.

        Show
        Petr Škoda added a comment - Hi, I was aware of this problem when writing the debug mode for themes, the problem here is that it was not intended for any production servers, it was designed only for developer installs with few requests for people writing new themes or debugging CSS problems.
        Hide
        ajeet added a comment -

        Hi
        getting Blank Page when try to insert a resource or activity in moodle 1.9.5

        Kindly help
        Also when i enable debugging, still getting blank page...

        Show
        ajeet added a comment - Hi getting Blank Page when try to insert a resource or activity in moodle 1.9.5 Kindly help Also when i enable debugging, still getting blank page...
        Hide
        Andrew Nicols added a comment -

        Hi ajeet,

        The problem you're experiencing is completely unrelated and for an out-of-date version of moodle which is no longer supported. See http://docs.moodle.org/dev/Releases#Moodle_1.9 for information on the support status of Moodle 1.9.

        Show
        Andrew Nicols added a comment - Hi ajeet, The problem you're experiencing is completely unrelated and for an out-of-date version of moodle which is no longer supported. See http://docs.moodle.org/dev/Releases#Moodle_1.9 for information on the support status of Moodle 1.9.
        Hide
        Petr Škoda added a comment -

        Thanks for the report, I have rewritten the code a bit to eliminate some race conditions, hopefully it will be more reliable now.

        Show
        Petr Škoda added a comment - Thanks for the report, I have rewritten the code a bit to eliminate some race conditions, hopefully it will be more reliable now.
        Hide
        Petr Škoda added a comment -

        I came to the same atomicity conclusion before, the rename() is already used in normal theme serving scripts.

        Show
        Petr Škoda added a comment - I came to the same atomicity conclusion before, the rename() is already used in normal theme serving scripts.
        Hide
        Dan Poltawski added a comment -

        Taking integration held issues out of integration (whilst we are keeping master and 23_STABLE in sync).

        Show
        Dan Poltawski added a comment - Taking integration held issues out of integration (whilst we are keeping master and 23_STABLE in sync).
        Hide
        Sam Hemelryk added a comment -

        Thanks Petr, the changes look spot on and I've integrated them now.
        Hopefully things will be a little more stable through that code path now.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Petr, the changes look spot on and I've integrated them now. Hopefully things will be a little more stable through that code path now. Cheers Sam
        Hide
        Rajesh Taneja added a comment -

        Assigning this test to Tim, as I don't have jmeter installed.

        Show
        Rajesh Taneja added a comment - Assigning this test to Tim, as I don't have jmeter installed.
        Hide
        Tim Barker added a comment -

        Great work, ran through the tests and everything works as expected.

        Show
        Tim Barker added a comment - Great work, ran through the tests and everything works as expected.
        Hide
        Dan Poltawski added a comment -

        Congratulations!

        You've made it into the weekly release!

        Thanks for your contribution - here are some random drummers to keep you inspired for the next week!
        http://www.youtube.com/watch?v=_QhpHUmVCmY

        Show
        Dan Poltawski added a comment - Congratulations! You've made it into the weekly release! Thanks for your contribution - here are some random drummers to keep you inspired for the next week! http://www.youtube.com/watch?v=_QhpHUmVCmY

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: