Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30349

Moodle does not clear file status cache when deleting directories and before creating theme cache files

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Libraries
    • Labels:

      Description

      Text rewritten by skodak:

      1/ David highlighted interesting hack in Typo3 recursive implementation - it manually resets file stat cache after deleting dirs, this may workaround some PHP bugs, we should steal it from there.

      2/ skodak found a potential caching problem (when reviewing original David's proposal to reset stat cache after crating dirs) when storing theme caches. The trouble is that different request may delete cache directories, solution is to reset file stat cache right before creating the dir structure for new cache files.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            mudrd8mz David Mudrak added a comment -

            I belive this would make MDL-30294 not appearing.

            Show
            mudrd8mz David Mudrak added a comment - I belive this would make MDL-30294 not appearing.
            Hide
            mudrd8mz David Mudrak added a comment -

            Adding wise men who might have an interesting option on this. RFC

            Show
            mudrd8mz David Mudrak added a comment - Adding wise men who might have an interesting option on this. RFC
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            format c: and done!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - format c: and done!
            Hide
            skodak Petr Skoda added a comment -

            I always thought that the clearstatcache() is useful when checking if dir exists and the file might have been changed in the meantime from another process. Any file deletion via PHP functions from the current request should ideally invalidate the stat cache properly - arrrggghhhhh!

            Show
            skodak Petr Skoda added a comment - I always thought that the clearstatcache() is useful when checking if dir exists and the file might have been changed in the meantime from another process. Any file deletion via PHP functions from the current request should ideally invalidate the stat cache properly - arrrggghhhhh!
            Hide
            skodak Petr Skoda added a comment -

            Hmm, clearing stat caches after creating dirs would not help, we would have to reset it before the is_dir() which would be very slow. So far the only place we could improve is our remove_dir() function.

            I am going to try a bit differect solution directly in theme/image.php and friends because we have the stale stat problems there right after cache resetting...

            Show
            skodak Petr Skoda added a comment - Hmm, clearing stat caches after creating dirs would not help, we would have to reset it before the is_dir() which would be very slow. So far the only place we could improve is our remove_dir() function. I am going to try a bit differect solution directly in theme/image.php and friends because we have the stale stat problems there right after cache resetting...
            Hide
            skodak Petr Skoda added a comment - - edited

            I am going to hijack this issue (I hope David does not mind, I owe you a beer for bringing this to my attention) and implement invalidation of file stat cache in our remove_dir() and invalidate the caches manually in our theme serving files. The original text was:

            ====================

            Moodle does not clear file status cache when creating new directories

            While trying to confirm MDL-30294 we discovered that check_dir_exists() creates new directories based on is_dir() check. However, is_dir() results are cached internally by PHP. It seems reasonable to call clearstatcache() just after we create new directory. At the moment, we call it only in purge_all_caches() and Typo3 calls it in its rmdir() method.

            Looking into setuplib.php we should call it in both places where we conditionally create directories: check_dir_exists() and make_writable_directory()

            Show
            skodak Petr Skoda added a comment - - edited I am going to hijack this issue (I hope David does not mind, I owe you a beer for bringing this to my attention) and implement invalidation of file stat cache in our remove_dir() and invalidate the caches manually in our theme serving files. The original text was: ==================== Moodle does not clear file status cache when creating new directories While trying to confirm MDL-30294 we discovered that check_dir_exists() creates new directories based on is_dir() check. However, is_dir() results are cached internally by PHP. It seems reasonable to call clearstatcache() just after we create new directory. At the moment, we call it only in purge_all_caches() and Typo3 calls it in its rmdir() method. Looking into setuplib.php we should call it in both places where we conditionally create directories: check_dir_exists() and make_writable_directory()
            Hide
            skodak Petr Skoda added a comment -

            I have proposed a different solution for MDL-30294 and sent it to integration.

            Show
            skodak Petr Skoda added a comment - I have proposed a different solution for MDL-30294 and sent it to integration.
            Hide
            mudrd8mz David Mudrak added a comment -

            Of course I don't mind But will invalidating on remove be enough? I don't know anything about how php caches files stats but let us consider the following:

            1. let us expect no cached data and the dir does not exist
            2. first call to check_dir_exists() runs is_dir(). That returns false and is cached
            3. the dir is created but the stat cache is not cleared (current behaviour)
            4. the next call to check_dir_exists() gets cached result of is_dir() and tries to create existing dir

            This is how I believed the reported issue emerged.

            Show
            mudrd8mz David Mudrak added a comment - Of course I don't mind But will invalidating on remove be enough? I don't know anything about how php caches files stats but let us consider the following: let us expect no cached data and the dir does not exist first call to check_dir_exists() runs is_dir(). That returns false and is cached the dir is created but the stat cache is not cleared (current behaviour) the next call to check_dir_exists() gets cached result of is_dir() and tries to create existing dir This is how I believed the reported issue emerged.
            Hide
            skodak Petr Skoda added a comment - - edited

            1/ file stat caching should be reliable when all file operations are done from one request. The rmdir() hack I introduced in patch may help if PHP was/is/will be borked. The delete and rename PHP operations should invalidate the stat cache automatically (at least PHP man says something like that), of course rm from shell exec breaks it.

            2/ PHP file operations break often when you delete and create the same dirs/files from two separate threads. The file stat caching makes it even worse. My proposed solution in the linked issue hides some errors and tries to eliminate others in some critical parts.

            3/ PHP manual says "You should also note that PHP doesn't cache information about non-existent files.", so I suppose your 2. is not valid.

            Show
            skodak Petr Skoda added a comment - - edited 1/ file stat caching should be reliable when all file operations are done from one request. The rmdir() hack I introduced in patch may help if PHP was/is/will be borked. The delete and rename PHP operations should invalidate the stat cache automatically (at least PHP man says something like that), of course rm from shell exec breaks it. 2/ PHP file operations break often when you delete and create the same dirs/files from two separate threads. The file stat caching makes it even worse. My proposed solution in the linked issue hides some errors and tries to eliminate others in some critical parts. 3/ PHP manual says "You should also note that PHP doesn't cache information about non-existent files.", so I suppose your 2. is not valid.
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks, this was cool. I didn't see any performance regressions here. I did wonder about remote (http) files but it doesn't apply to themes. remove_dir()'s recursion looks good too.

            Its been integrated to master.

            Show
            nebgor Aparup Banerjee added a comment - Thanks, this was cool. I didn't see any performance regressions here. I did wonder about remote (http) files but it doesn't apply to themes. remove_dir()'s recursion looks good too. Its been integrated to master.
            Hide
            nebgor Aparup Banerjee added a comment -

            pushing this to pass - too hard to test think of a test even

            Show
            nebgor Aparup Banerjee added a comment - pushing this to pass - too hard to test think of a test even
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  5/Dec/11