Right now, when a stage is destroyed, it is completely deleted right then and there. That takes a little bit of time, which might be better spent doing other things.
This also would speed thing up from the users perspective because now in Automatic Update forms and Project Browser the operation is not complete until we delete the directory. So we are making the user wait for this when they really don't need to.
The StageBase::destroy() method should mark a directory for deletion, and Package Manager should clean it up during cron later. Any files that cannot be deleted should just be ignored, on the assumption that the operating system will do that clean-up on its own.
We could mark the stage directory for deletion by adding it to a queue, and letting cron handle it on its own.
StageBase::__construct() will need Drupal\Core\Queue\QueueFactory where it currently takes FileSystemInterface. So this is a breaking change for anything that extends StageBase (hello, Project Browser!)
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | package_manager_test_event.json_.txt | 1.45 KB | tedbow |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
tedbowIf we do this issue do we really need the PreDestroy and PostDestroy events?
Comment #5
omkar.podey commentedNeeds more info on how exactly this will work does the cron operation just search all instances of
DELETE_stagein case of/private/tmp/package_manager_testing_roottest64053336/stagethe directory to be searched in is according to the site settings and just starts recursively deleting all things inside of it ? OR should we somehow tell the cron specifically what directory needs to be cleaned up.Won't the stage directories pile up if cron isn't run for a long time ? do we need to make this optional ?
Comment #6
tedbowComment #7
tedbowI did some simple benchmarking in #3381484: In package_manager_test_event_logger also log the time to help benchmarking and debugging . It looks like deleting the directory does not take take much time compared to the other stages. 4 secs.
So the 4 seconds is better but not a huge improvement. I wonder if the bigger advantage would be removing the PreDestroy and PostDestory events. What would be use cases for using these vs PostApply.
Comment #8
phenaproximaIf we remove both of them, I'd be comfortable removing PreDestroyEvent and PostDestroyEvent. That would indeed be simpler, and I can't think of a case for them that can't be done in post-apply.
Comment #9
tedbowre #5
I think would just delete everything inside of
\Drupal\package_manager\PathLocator::getStagingRootthat starts withDELETE_Cron is expected to be run regularly or your site might not function correctly. So I don't think we need to make it optional
though maybe with project browser I could many stage life cycles before cron is run.
Another option we could do is set a set state flag in
\Drupal\package_manager\StageBase::destroyand then createStageCleanUpservice that subscribes to\Symfony\Component\HttpKernel\KernelEvents::TERMINATEand checks the flag and then deletes any "DELETE_" directoriesComment #10
tedbowLooking good. Needs work for MR comments
Comment #11
phenaproximaComment #12
phenaproximaComment #13
phenaproximaComment #14
tedbowLooking good. Just a couple small things
Comment #15
phenaproximaComment #16
phenaproximaComment #17
phenaproximaComment #18
tedbowComment #19
phenaproximaComment #20
tedbowlooks good.
Looks like 3.0.x needs to be merged in again but otherwise should be good if tests pass again
Comment #21
phenaproximaComment #23
phenaproxima