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

Fix log writing failed. can't be called from trap context (no thread version) #1502

Closed
wants to merge 4 commits into from

Conversation

sonots
Copy link

@sonots sonots commented Aug 22, 2016

Fix #1493, similarly with #1499, but this is no thread version.

Must restart thread since fork does not copy a thread
Must reopen pipe, or parent process would receive a signal
@@ -228,7 +228,7 @@ def work(interval = 5.0, &block)
break if interval.zero?
log_with_severity :debug, "Sleeping for #{interval} seconds"
procline paused? ? "Paused" : "Waiting for #{queues.join(',')}"
sleep interval
@signal_handler.handle_signal(interval)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the other PR, this will only run if the queue was empty. Should we move this out of the unless?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, if we move this out of the unless, waiting interval is issued even if we run a job (behavior changes).
I am not getting good idea yet. I will think of how to resolve this. Any idea is welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do a non-blocking select so it doesn't sleep and just returns if there was no signal yet? and keep the sleep here where it was

Copy link
Author

@sonots sonots Aug 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied a change like 4ac5b36.

can we do a non-blocking select so it doesn't sleep and just returns if there was no signal yet? and keep the sleep here where it was

During sleeping, we can not get signal, then. I felt @signal_handler.handle_signal(interval) here is still better, but I added @signal_handler.handle_signal(0) for the case work_one_job returns true.

Also, I added handle_signal in perform_with_fork for during waiting a forked child finishes.

Please note that we still have problems that we can not handle signal if fork_per_job? is false. Hmm.

@coveralls
Copy link

coveralls commented Aug 22, 2016

Coverage Status

Coverage decreased (-0.2%) to 82.897% when pulling e994f20 on sonots:fix_signal_trap2 into 651a532 on resque:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 82.897% when pulling e994f20 on sonots:fix_signal_trap2 into 651a532 on resque:master.

@coveralls
Copy link

coveralls commented Aug 22, 2016

Coverage Status

Coverage decreased (-0.2%) to 82.908% when pulling 6f28013 on sonots:fix_signal_trap2 into 651a532 on resque:master.

@coveralls
Copy link

coveralls commented Aug 22, 2016

Coverage Status

Coverage decreased (-0.09%) to 83.001% when pulling 4ac5b36 on sonots:fix_signal_trap2 into 651a532 on resque:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 83.001% when pulling 4ac5b36 on sonots:fix_signal_trap2 into 651a532 on resque:master.

unless work_one_job(&block)
if work_one_job(&block)
@signal_handler.handle_signal(0)
else
break if interval.zero?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line here might mess up your logic.. if the interval is zero and the queue is empty, then we wont handle signals...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for pointing out

@sonots
Copy link
Author

sonots commented Aug 22, 2016

We still have problems that we can not handle signal during processing a job when fork_per_job? returns false. Thread vesion does not have such an issue.

@sonots
Copy link
Author

sonots commented Aug 23, 2016

As a final thought, there is no way to resolve the above issue with this no thread version. Let me close.

@sonots sonots closed this Aug 23, 2016
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.

None yet

3 participants