Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Worker pcntl_wait to non-blocking loop #189

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Worker pcntl_wait to non-blocking loop #189

wants to merge 2 commits into from

Conversation

Rockstar04
Copy link
Contributor

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 Rockstar04 changed the title Move wait to a look Worker pcntl_wait to non-blocking loop May 14, 2014
Putting the thread to sleep with wait no longer allows signals to be captured. This allow signals like USR1 to be used again

Correct Parse error in Worker

Finally currect parse error
@Rockstar04
Copy link
Contributor Author

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

@danhunsaker
Copy link
Contributor

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
Copy link
Contributor Author

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

Sent from my Droid 4

@danhunsaker
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@danhunsaker
Copy link
Contributor

This is a good question...

danhunsaker added a commit to resque/php-resque that referenced this pull request Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants