Moodle
  1. Moodle
  2. MDL-36363

Removing a file store cache instance should remove the folder too, at least remove the files representing the cached items

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4, 2.5
    • Fix Version/s: 2.4.2
    • Component/s: Caching
    • Testing Instructions:
      Hide

      Test 1: Test functionality.

      1. Open Site administration ► Plugins ► Caching ► Configuration
      2. Create a new file store (Add instance at the end of the row identified by column Plugin = File cache) in a Cache path really different from the default one e.g. /tmp/moodle-master-cache-file1
      3. Edit the Language string cache mapping and point it to the new file store
      4. Edit the Language string cache mapping and point it to the default store
      5. Check that cache files have been created as expected e.g. ls /tmp/moodle-master-cache-file1/*/.cache
      6. Remove the new file store
      7. Check that *.cache files have been removed from the folder used as the Cache path for the new file store
      8. Create a dummy cache file in the folder above e.g. touch /tmp/moodle-master-cache-file1/core_string/en_/TEST_DUMMY_KEY.cache keeping care of the R/W permission for the WebServer user
      9. Re-create the new file store with the same settings
      10. Check that the dummy cache file has been removed.

      Test 2: Test stores via performance.

      1. Install the following php extensions if you don't already have them: memcache, memcached, and mongodb.
      2. Configure two running instances of memcache.
      3. Browse to Settings > Plugins > Caches > Cache stores > Memcache and set up a test instance using the first running memcache.
      4. Browse to Settings > Plugins > Caches > Cache stores > Memcached and set up a test instance using the second running memcache.
      5. Browse to Settings > Plugins > Caches > Cache stores > Mongodb and set up the test instance.
      6. Browse to Settings > Plugins > Caches > Test performance.
      7. Check that all stores complete without error.
      Show
      Test 1: Test functionality. Open Site administration ► Plugins ► Caching ► Configuration Create a new file store ( Add instance at the end of the row identified by column Plugin = File cache ) in a Cache path really different from the default one e.g. /tmp/moodle-master-cache-file1 Edit the Language string cache mapping and point it to the new file store Edit the Language string cache mapping and point it to the default store Check that cache files have been created as expected e.g. ls /tmp/moodle-master-cache-file1/* / .cache Remove the new file store Check that *.cache files have been removed from the folder used as the Cache path for the new file store Create a dummy cache file in the folder above e.g. touch /tmp/moodle-master-cache-file1/core_string/en_/TEST_DUMMY_KEY.cache keeping care of the R/W permission for the WebServer user Re-create the new file store with the same settings Check that the dummy cache file has been removed. Test 2: Test stores via performance. Install the following php extensions if you don't already have them: memcache, memcached, and mongodb. Configure two running instances of memcache. Browse to Settings > Plugins > Caches > Cache stores > Memcache and set up a test instance using the first running memcache. Browse to Settings > Plugins > Caches > Cache stores > Memcached and set up a test instance using the second running memcache. Browse to Settings > Plugins > Caches > Cache stores > Mongodb and set up the test instance. Browse to Settings > Plugins > Caches > Test performance. Check that all stores complete without error.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull 2.4 Branch:
      wip-MDL-36363-m24
    • Pull Master Branch:
      wip-MDL-36363-m25
    • Rank:
      45172

      Description

      If you remove the file store and you've created it with the "autocreate" option, the expected result is that the folder configured as "path" will be removed too.
      Besides before deleting a store it should be purged.

      For safety reasons, e.g. when using system folders with "autocreate", option it is wise to purge cached items leaving the folder there.

        Issue Links

          Activity

          Hide
          Matteo Scaramuccia added a comment -

          I'm not sure about my patch proposal, that's the reason why the branch name starts w/ wip_.

          I'm thinking about the reason to create an ad hoc definition which actually affects the purge operation by creating an extra subfolder under 'path' (ref.: cachestore_file::initialise()): my proposal doesn't get the expected result due to that extra subfolder.
          Besides that subfolder is named adhoc_core_cache_purge and I'm not able right now (started today to look at the MUC code) to understand if purging a file store will ever work if the file store cache instance is initialised in a "normal way".

          Show
          Matteo Scaramuccia added a comment - I'm not sure about my patch proposal, that's the reason why the branch name starts w/ wip_ . I'm thinking about the reason to create an ad hoc definition which actually affects the purge operation by creating an extra subfolder under 'path' (ref.: cachestore_ file::initialise( ) ): my proposal doesn't get the expected result due to that extra subfolder. Besides that subfolder is named adhoc_core_cache_purge and I'm not able right now (started today to look at the MUC code) to understand if purging a file store will ever work if the file store cache instance is initialised in a "normal way".
          Hide
          Matteo Scaramuccia added a comment -

          I'm pretty sure that https://github.com/scara/moodle/commit/5a26d449a73d1130a2dfa78471db2930478a6c24#L1R290 is not correct: it gets into the store logics without abstraction.

          What about adding a new bool cache_store ::destroy() method which implements destruction on per-store basis? Besides a new property, hash, should be added in cachestore_file to cleanup the implementation of initialise, improving a possible implementation of destroy() charged to remove anything under path.

          I'm guessing if this kind of re-working should wait for the implementation of MDL-36120.

          Show
          Matteo Scaramuccia added a comment - I'm pretty sure that https://github.com/scara/moodle/commit/5a26d449a73d1130a2dfa78471db2930478a6c24#L1R290 is not correct: it gets into the store logics without abstraction. What about adding a new bool cache_store ::destroy() method which implements destruction on per-store basis? Besides a new property, hash , should be added in cachestore_file to cleanup the implementation of initialise , improving a possible implementation of destroy() charged to remove anything under path . I'm guessing if this kind of re-working should wait for the implementation of MDL-36120 .
          Hide
          Matteo Scaramuccia added a comment -

          See also MDL-36512: I'll work on this using that proposal as the starting point.
          Whyle studying MUC code I've found that my proposal of destroy could be based on cleanup which stops at the definition level in removing folders - imposed with initialise - or, maybe, to remove the base path of the file store just after actually purging the store (MDL-36512).

          Show
          Matteo Scaramuccia added a comment - See also MDL-36512 : I'll work on this using that proposal as the starting point. Whyle studying MUC code I've found that my proposal of destroy could be based on cleanup which stops at the definition level in removing folders - imposed with initialise - or, maybe, to remove the base path of the file store just after actually purging the store ( MDL-36512 ).
          Hide
          Matteo Scaramuccia added a comment -

          I've modified my first proposal, https://github.com/scara/moodle/commit/5a26d449a73d1130a2dfa78471db2930478a6c24 in branch wip_m24MDL-36363Remove_path_when_removing_a_file_store_cache.
          I keep on wondering if it could be considered AS-IS without abstracting the explicit destruction required for just a file store.

          Show
          Matteo Scaramuccia added a comment - I've modified my first proposal, https://github.com/scara/moodle/commit/5a26d449a73d1130a2dfa78471db2930478a6c24 in branch wip_m24 MDL-36363 Remove_path_when_removing_a_file_store_cache . I keep on wondering if it could be considered AS-IS without abstracting the explicit destruction required for just a file store.
          Hide
          Sam Hemelryk added a comment -

          Hi Matteo,

          Thanks for the creating reporting the issue and providing a patch.
          Certainly 100% this is a good improvement.
          Having looked at the patch I think a better approach would be to ask the store to clean itself up before being deleted rather than addressing the file store personally.
          The reason for this being that other stores, memcache, apc, database whatever will also want a chance to clean up.
          What do you think of the idea of implementing two new methods against the cache_store interface:

          • instance_created : called when a store instance is first created.
          • instance_deleted : called when the store instance is deleted.

          If you like the idea and would be happy to implement it I will leave it up for you, otherwise I can jump on this one myself.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Matteo, Thanks for the creating reporting the issue and providing a patch. Certainly 100% this is a good improvement. Having looked at the patch I think a better approach would be to ask the store to clean itself up before being deleted rather than addressing the file store personally. The reason for this being that other stores, memcache, apc, database whatever will also want a chance to clean up. What do you think of the idea of implementing two new methods against the cache_store interface: instance_created : called when a store instance is first created. instance_deleted : called when the store instance is deleted. If you like the idea and would be happy to implement it I will leave it up for you, otherwise I can jump on this one myself. Many thanks Sam
          Hide
          Matteo Scaramuccia added a comment -

          Hi Sam,
          it sounds good - kind of BC, is it possible when being in beta? For sure there are good reasons -: if you have enough time (not busy for other urgent tasks for 2.4beta) and you think it is urgent, please jump on it.
          I'm happy in contributing but I must be honest: I'm a bit slow not only for lacking of spare time and not being a subject matter expert but for flu. Being Friday, time for integration has gone and probably my slowness shouldn't be an issue if a proposal will land the next week.

          Show
          Matteo Scaramuccia added a comment - Hi Sam, it sounds good - kind of BC, is it possible when being in beta? For sure there are good reasons -: if you have enough time (not busy for other urgent tasks for 2.4beta) and you think it is urgent, please jump on it. I'm happy in contributing but I must be honest: I'm a bit slow not only for lacking of spare time and not being a subject matter expert but for flu. Being Friday, time for integration has gone and probably my slowness shouldn't be an issue if a proposal will land the next week.
          Hide
          Matteo Scaramuccia added a comment -

          Fever has gone , I've updated the branch with your proposal BUT I'm not confident at all about MY first proposal with a full deletion so I'll improve it with another commit to restrict the deletion to what the file store is supposed to create, regardless the specific definition.
          Then obviously up to you to define what's better, even to keep your idea about the two new method but w/o doing anything in file store too i.e. a third commit, the safer moving this issue as just an improvement of the API for future use.

          Show
          Matteo Scaramuccia added a comment - Fever has gone , I've updated the branch with your proposal BUT I'm not confident at all about MY first proposal with a full deletion so I'll improve it with another commit to restrict the deletion to what the file store is supposed to create, regardless the specific definition. Then obviously up to you to define what's better, even to keep your idea about the two new method but w/o doing anything in file store too i.e. a third commit, the safer moving this issue as just an improvement of the API for future use.
          Hide
          Matteo Scaramuccia added a comment - - edited

          Pushed:
          a. Commit 1+2: my proposal
          b. Commit 1+2+3: the safest option, with two small improvement: $filestorepath and purge() implementation in cachestore_file, to play its role when path has been defined.

          Squashing will be required for both options.

          Show
          Matteo Scaramuccia added a comment - - edited Pushed: a. Commit 1+2 : my proposal b. Commit 1+2+3 : the safest option, with two small improvement: $filestorepath and purge() implementation in cachestore_file, to play its role when path has been defined. Squashing will be required for both options.
          Hide
          Matteo Scaramuccia added a comment -

          Fixed a missing TNX to PHPunit (phpunit core_cache cache/tests/cache_test.php) telling about the missing instance_*() implementations in dummystore.php, cachestore_dummy.

          Show
          Matteo Scaramuccia added a comment - Fixed a missing TNX to PHPunit ( phpunit core_cache cache/tests/cache_test.php ) telling about the missing instance_*() implementations in dummystore.php , cachestore_dummy.
          Hide
          Matteo Scaramuccia added a comment - - edited

          Hi Sam,
          back again, while looking at MDL-36322 I found an issue with file store:

          ...
                      if ($path !== false && !is_writable($path)) {
                          $path = false;
          ...
                  $this->isready = $path !== false;
          ...
              public function is_ready() {
                  return ($this->path !== null);
              }
          ...
          

          which means that is_ready will not tells about any failure during the creation of the instance when attempting to create the path if required. I noticed it just now, when looking for a reason for MDL-36322 and looking at my proposal here where I've used the property and not the function.
          Since this is really a separate issue do I need to open a new one or it could be added here or (IMHO better) in MDL-36322?

          EDIT@20121128T0732CEST: the is_ready() issue has been fixed in MDL-36825.

          Show
          Matteo Scaramuccia added a comment - - edited Hi Sam, back again, while looking at MDL-36322 I found an issue with file store: ... if ($path !== false && !is_writable($path)) { $path = false ; ... $ this ->isready = $path !== false ; ... public function is_ready() { return ($ this ->path !== null ); } ... which means that is_ready will not tells about any failure during the creation of the instance when attempting to create the path if required. I noticed it just now, when looking for a reason for MDL-36322 and looking at my proposal here where I've used the property and not the function. Since this is really a separate issue do I need to open a new one or it could be added here or (IMHO better) in MDL-36322 ? EDIT@20121128T0732CEST: the is_ready() issue has been fixed in MDL-36825 .
          Hide
          Sam Hemelryk added a comment -

          Hi Matteo,

          Looks like my time today has already been filled sorry but I'll hopefully look to this tomorrow and will have some feedback/patch to share.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Matteo, Looks like my time today has already been filled sorry but I'll hopefully look to this tomorrow and will have some feedback/patch to share. Many thanks Sam
          Hide
          Matteo Scaramuccia added a comment -

          Rebased against 2.4rc1 and cleaned up using a different set of commits no more linked to the strategy used to present the proposal in the "old" branch, m24_MDL-36363_Remove_path_when_removing_a_file_store_cache: now the commits history should be preserved.

          Show
          Matteo Scaramuccia added a comment - Rebased against 2.4rc1 and cleaned up using a different set of commits no more linked to the strategy used to present the proposal in the "old" branch, m24_ MDL-36363 _Remove_path_when_removing_a_file_store_cache: now the commits history should be preserved.
          Hide
          Sam Hemelryk added a comment -

          Hi Matteo, sorry it took me so long to get to these issues.
          I'm onto them today.

          Show
          Sam Hemelryk added a comment - Hi Matteo, sorry it took me so long to get to these issues. I'm onto them today.
          Hide
          Sam Hemelryk added a comment -

          Thanks Matteo.

          I've looked this over and having thought about it for quite some time I've made a couple of changes.
          Rather than have both cleanup and instance_deleted methods I opted for just instance_deleted. Reading back over comments and documentation I am sure that cleanup was intended to clean up deleted stores anyway.
          I also fixed up the memcache and mongodb instance_deleted methods so that they would work if the cache had not been initialised.
          I've made my changes as an additional change on top of your and am putting this up for integration review now.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Matteo. I've looked this over and having thought about it for quite some time I've made a couple of changes. Rather than have both cleanup and instance_deleted methods I opted for just instance_deleted. Reading back over comments and documentation I am sure that cleanup was intended to clean up deleted stores anyway. I also fixed up the memcache and mongodb instance_deleted methods so that they would work if the cache had not been initialised. I've made my changes as an additional change on top of your and am putting this up for integration review now. Many thanks Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Only one tiny detail... if we mark the function as deprecated for 2.5 ... shouldn't it be calling the newer one instead of debugging and doing nothing? I mean, that's not deprecation, it's kill.

          Show
          Eloy Lafuente (stronk7) added a comment - Only one tiny detail... if we mark the function as deprecated for 2.5 ... shouldn't it be calling the newer one instead of debugging and doing nothing? I mean, that's not deprecation, it's kill.
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          It is sort of kill, essentially it won't cause any harm it just won't purge the cache.
          The only people affected would be those who would have instantiated a cache store instance themselves and have written code to call cleanup manually. Hopefully (and I imagine to be the case) no one has done that.
          Instead I opted to make instance_deleted call cleanup if it exists. This I feel is a much better option, I'm sure others have created cache store plugins and have written cleanup methods. This way existing cache stores won't be break, however they will see a debugging notice.
          It also means however that I can't make cleanup call instance_deleted... not easily anyway.
          I'd rather this stays as is but am happy to look at other solutions if you desire.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, It is sort of kill, essentially it won't cause any harm it just won't purge the cache. The only people affected would be those who would have instantiated a cache store instance themselves and have written code to call cleanup manually. Hopefully (and I imagine to be the case) no one has done that. Instead I opted to make instance_deleted call cleanup if it exists. This I feel is a much better option, I'm sure others have created cache store plugins and have written cleanup methods. This way existing cache stores won't be break, however they will see a debugging notice. It also means however that I can't make cleanup call instance_deleted... not easily anyway. I'd rather this stays as is but am happy to look at other solutions if you desire. Many thanks Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Alright, integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Alright, integrated, thanks!
          Hide
          Rossiani Wijaya added a comment -

          while testing:

          Step 4: First time I test this issue, I get: Fatal error: Allowed memory size of 2097152000 bytes exhausted (tried to allocate 76 bytes) in /integration/24/cache/classes/helper.php on line 476 for for both Master and 2.4
          Then I re-run the test again by removing all folders within the cache folder in moodledata, I didn't get the above error, however there's no theme applied for the page.

          Then when I continue on to step 5 and noticed that I didn't get any other file/folder except 'core string' folder.

          Could you confirm that the above error and behaviour are the expected result, before I continue on to the next step?

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - while testing: Step 4: First time I test this issue, I get: Fatal error: Allowed memory size of 2097152000 bytes exhausted (tried to allocate 76 bytes) in /integration/24/cache/classes/helper.php on line 476 for for both Master and 2.4 Then I re-run the test again by removing all folders within the cache folder in moodledata, I didn't get the above error, however there's no theme applied for the page. Then when I continue on to step 5 and noticed that I didn't get any other file/folder except 'core string' folder. Could you confirm that the above error and behaviour are the expected result, before I continue on to the next step? Thanks Rosie
          Hide
          Sam Hemelryk added a comment -

          Thanks for reporting that Rosie.

          I tracked it down this morning to the cache key being parsed multiple times if there are stacked loaders (you've mapped two stores to a single definition).
          I've created a fix for this that is now ready for integration:

          Branches:

          Diffs

          All your's once more Eloy.

          Thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for reporting that Rosie. I tracked it down this morning to the cache key being parsed multiple times if there are stacked loaders (you've mapped two stores to a single definition). I've created a fix for this that is now ready for integration: Branches: wip- MDL-36363 -m24-integration wip- MDL-36363 -m25-integration Diffs https://github.com/samhemelryk/moodle/commit/wip-MDL-36363-m24-integration https://github.com/samhemelryk/moodle/commit/wip-MDL-36363-m25-integration All your's once more Eloy. Thanks Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Pushed the extra 2 commits. Now this can be tested again and verify the memory problem doesn't happen anymore and everything works as expected.

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Pushed the extra 2 commits. Now this can be tested again and verify the memory problem doesn't happen anymore and everything works as expected. Thanks!
          Hide
          Rossiani Wijaya added a comment -

          Re-tested this issue with the updated patch.

          It works as expected for 2.4 and master

          Test passed.

          Show
          Rossiani Wijaya added a comment - Re-tested this issue with the updated patch. It works as expected for 2.4 and master Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao
          Hide
          Michael de Raadt added a comment -

          I've removed the dev_docs_required label. The API still needs to be documented as part of MDL-25290.

          Show
          Michael de Raadt added a comment - I've removed the dev_docs_required label. The API still needs to be documented as part of MDL-25290 .

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: