Bugfix: workers on non-forking OSs read jobs from the queue, but never actually do any work on them. #93

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@Crashthatch

Change from $this->fork() to Resque::fork() in 6800fbe now returns -1 (not false) and this should be checked for.

Bugfix for non-forking OSs where workers take from the queue, but nev…
…er actually do any work.

Change from $this->fork() to Resque::fork() in 6800fbe now returns -1 (not false) and this should be checked for.
@danhunsaker

This comment has been minimized.

Show comment
Hide comment
@danhunsaker

danhunsaker May 10, 2013

Contributor

Seems to me that this would only be an issue on Windows or certain mobile/embedded devices - and I would hardly consider either of those ideal environments for anything near this intense. Virtual machine technology has come a great distance in recent years, and can easily provide access to a properly-forking OS on any system with a minimum of overhead. It may take some work to get your system properly set up for this kind of project, but that amount of work will still probably be less, in the long run, than that required to work around the limitations of Windows and embedded devices just to get something working.

Contributor

danhunsaker commented May 10, 2013

Seems to me that this would only be an issue on Windows or certain mobile/embedded devices - and I would hardly consider either of those ideal environments for anything near this intense. Virtual machine technology has come a great distance in recent years, and can easily provide access to a properly-forking OS on any system with a minimum of overhead. It may take some work to get your system properly set up for this kind of project, but that amount of work will still probably be less, in the long run, than that required to work around the limitations of Windows and embedded devices just to get something working.

@richardkmiller

This comment has been minimized.

Show comment
Hide comment
@richardkmiller

richardkmiller Jun 19, 2013

Contributor

+1 for this PR. I closed #94 because this one was here first and handles my case perfectly.

Contributor

richardkmiller commented Jun 19, 2013

+1 for this PR. I closed #94 because this one was here first and handles my case perfectly.

@chrisboulton

This comment has been minimized.

Show comment
Hide comment
@chrisboulton

chrisboulton Aug 16, 2016

Owner

I don't think I can merge this PR as it stands, because it will introduce a behavioural difference. The actual fix is to fix the return value of Resque::fork, which I'll take care of separately.

The problem with doing merging this PR as it currently stands is that is that on a forking OS that has run out of file descriptors, or is broken in some other way where the fork operation fails (and you should be alerted to it as failing), this code will now begin executing jobs inline.

Owner

chrisboulton commented Aug 16, 2016

I don't think I can merge this PR as it stands, because it will introduce a behavioural difference. The actual fix is to fix the return value of Resque::fork, which I'll take care of separately.

The problem with doing merging this PR as it currently stands is that is that on a forking OS that has run out of file descriptors, or is broken in some other way where the fork operation fails (and you should be alerted to it as failing), this code will now begin executing jobs inline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment