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?

CommentFileSizeAuthor
#1 1189696_autorestart.patch1.84 KBanarcat

Comments

anarcat’s picture

Status: Active » Needs review
StatusFileSize
new1.84 KB

Okay, 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...

steven jones’s picture

Would it be simpler to call drush_backend_fork with the correct arguments?

steven jones’s picture

Priority: Minor » Normal
Status: Needs review » Needs work

I 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.

steven jones’s picture

Status: Needs work » Fixed

I've pushed a fix into git, you'll be wanting to run the command using the new --auto-restart option, which will fork the process at the end of execution.

Good luck killing the process when you need to :)

steven jones’s picture

Status: Fixed » Needs work

This now needs documenting in the README

anarcat’s picture

The 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()...

anarcat’s picture

I 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.

anarcat’s picture

We'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). ;)

anarcat’s picture

Status: Needs work » Needs review

The readme is updated, waiting for steven's go before this big merge. :)

anarcat’s picture

Note 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...

steven jones’s picture

Status: Needs review » Needs work

I'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.

steven jones’s picture

Status: Needs work » Needs review

Anarcat, 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.

anarcat’s picture

I can merge your changes today to see if that works. I agree with removing the fork.

anarcat’s picture

Status: Needs review » Fixed

merged in 1.x

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.