Okay first off, just to clear that up - THIS IS AWESOME! :) Just watching the tasks get fired away like this is great and feels good.
Now, I have a problem: in lenny, the supervisor package doesn't exist. I know, I know, I need to upgrade, but that means PHP 5.3, and we're not quite there yet. So I need to find an alternative. One thing I thought of was to just let it run without restarting it.
But this means it gets in the dreaded "PHP leaks memory" problem:
Starting Hosting queue runner, watching for available tasks [1.23 sec, 44.54 MB] [notice]
Found task to execute. Pausing before execution. [2.08 sec, 47.24 MB] [notice]
[...] running tasks for 3 minutes
Finished executing task. [56.58 sec, 48.32 MB] [notice]
Roughly, 1MB / 3 minutes running tasks fully. In fact, every time a task is ran, PHP eats a whopping 100KB.
This sucks, but it's not this modules fault.
However, there may be something that can be done without reverting to using an external daemon (and requiring root). Something like Drupal's batch API (but we can't use that since it relies on Apache and so on).
A very simple thing would be to have a timer that, when it fires, the task execs itself. exec(), or pcntl-exec() as PHP mistakenly calls this noble function call, would be pretty much all we need.
Then the only thing we need to deal with are PHP segfaults, which maybe we can assume won't happen. Or we can have a crontab that will restart the daemon if it's dead.
In other words, what I am proposing is to patch this daemon to be self-restarting instead of relying on a third party to do that.
How does that sound?
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 1189696_autorestart.patch | 1.84 KB | anarcat |
Comments
Comment #1
anarcat commentedOkay, with the attached patch, the daemon automatically restarts after the delay.
To test this patch, I strongly recommend adding #1189768: customizable process lifetime first so that you can tweak the delay.
You need to enable this feature in the settings of the moduel before it works. But it works!
again, the only thing that will fail here is when the PHP process crashes...
Comment #2
steven jones commentedWould it be simpler to call
drush_backend_forkwith the correct arguments?Comment #3
steven jones commentedI think it makes more sense to use a drush command line option to specify the auto-restart functionality, rather than making it a variable. It's quite fundamental to how you might run the command, and not really config.
Comment #4
steven jones commentedI've pushed a fix into git, you'll be wanting to run the command using the new
--auto-restartoption, which will fork the process at the end of execution.Good luck killing the process when you need to :)
Comment #5
steven jones commentedThis now needs documenting in the README
Comment #6
anarcat commentedThe committed code will not avoid a memory leak. It will just fork a new process and the original process will just stay lying around. I think the original code should stay.
Also note that I have struggled with drush backend stuff to make it do my bidding, but somehow it's incapable of replacing the current process image in the way pcntl_exec() does. In fact, it's not even capable of generating a proper commandline that could be used by pcntl_exec()...
Comment #7
anarcat commentedI am working in a separate branch now, I have pushed it to the main repo (autorestart), thanks Steven for the commit access!
I have opened a feature request upstream so that the pcntl_exec() stuff is factored in: #1190822: pcntl_exec() support for invoke commands.
I'll test this branch in prod now.
Comment #8
anarcat commentedWe've successfully survived the onslaught of mass migration on our production server. I would like to merge the autostart branch in now, and I think this would warrant a release (when the README is updated of course). ;)
Comment #9
anarcat commentedThe readme is updated, waiting for steven's go before this big merge. :)
Comment #10
anarcat commentedNote that i pinged the waiting_queue people so that they merge those changes in too: #1191164: merge improvements from hosting_queue_runner.
Too bad we're not using Drupal queues yet - I guess that would be good 2.x material...
Comment #11
steven jones commentedI've cleaned up some of the code, and reworked how we work out what command to restart.
I've removed the forking, as I'm assuming from our conversation yesterday that if running under supervisor and we fork, that will cause supervisor to start us again.
Comment #12
steven jones commentedAnarcat, can you make sure the branch still works for you?
I'll make sure it still works for me in my use-cases, and if it does, I'll merge it in.
Comment #13
anarcat commentedI can merge your changes today to see if that works. I agree with removing the fork.
Comment #14
anarcat commentedmerged in 1.x