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 Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Libraries
    • Labels:
    • Rank:
      32717

      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.

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

          I belive this would make MDL-30294 not appearing.

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

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

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

          format c: and done!

          Show
          Eloy Lafuente (stronk7) added a comment - format c: and done!
          Hide
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - I have proposed a different solution for MDL-30294 and sent it to integration.
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - pushing this to pass - too hard to test think of a test even
          Hide
          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
          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: