search

chrisboulton / php-resque

PHP port of resque (Workers and Queueing)
MIT License
3.43k stars 759 forks source link

Worker pcntl_wait to non-blocking loop #189

Open Rockstar04 opened 10 years ago

Rockstar04 commented 10 years ago

Putting the thread to sleep with pcntl_wait no longer allows signals to be captured (PHP 5 >= 5.3.0). This PR puts pcntl_wait into a while loop waiting for the child to exit, and allows signals like USR1 to be used again (pcntl_signal_dispatch being called inside the loop).

My only concern would be compatibility with PHP< 5.3.0

http://www.php.net/manual/en/function.pcntl-signal.php#114575 http://www.php.net/manual/en/function.pcntl-signal-dispatch.php

Rockstar04 commented 10 years ago

It looks like #83 could also solve this problem, but I have not personally tested that PR

danhunsaker commented 10 years ago

I have tested #83, heavily, but that's beside the point.

My concern with this is that a while loop for this will quickly consume CPU, as there's nothing to keep the loop from moving as quickly as it can. Pegging the CPU in the worker would prevent the child from having any CPU to work with, causing all kinds of problems for the jobs. I'm sure that didn't happen in your tests, but this is the kind of thing that is heavily system-dependent, so it could easily be a problem in a different configuration.

It'll be tricky to figure out a better workaround... Honestly, ignoring signals during pcntl_wait sounds like a pretty big bug to me, so it would be preferable for the pcntl extension to be fixed instead, but we'll still need to work around the broken version(s).

As to #83, it's only real problem is that there are too many things I fixed in a single PR. And now I'd have to rebase it to get it working again.

Rockstar04 commented 10 years ago

Would a micro sleep like you used in #83 be an acceptable hack then?

Sent from my Droid 4

danhunsaker commented 10 years ago

Email replies to GitHub issues are UGLY...

Yes, given that's the entire reason I put the micro sleep in there to begin with. :-)

Rockstar04 commented 10 years ago

Sorry I wasn’t trying to imply #83 was not well tested, only that I had not confirmed if it properly responded to signals.

I just pushed a commit that adds a half second delay in checking for new signals

Rockstar04 commented 10 years ago

Are jobs that are killed with a USR1 supposed to be added to the failed job queue?

danhunsaker commented 8 years ago

This is a good question...