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

Signal processing improvement #66

Closed
wants to merge 1 commit into
base: master
from

Conversation

2 participants
@2miksyn
Contributor

2miksyn commented Apr 11, 2018

According python docs Python signal handlers are always executed in the main Python thread, but sometimes this is not true. This problem is too old, and on some OSs it periodically take place.
FreeBSD 10.3 amd64 send signal for last python thread, and it is not processed. FreeBSD 11.0 is not affected. Problem occurs on python 3.6 and 3.8.
Blocking signal processing in thread resolves problem independently of OS. Currently on some OSs dramatiq process cannot be killed by TERM and reloaded by HUP.

Signal processing improvement
According `python` docs `Python signal handlers are always executed in the main Python thread`, but sometimes this is not true. This problem is too old, and on some OSs it periodically take place.
FreeBSD 10.3 amd64 send signal for last python thread, and it is not processed. FreeBSD 11.0 is not affected. Problem occurs on python 3.6 and 3.8.
Blocking signal processing in thread resolves problem independently of OS.
@Bogdanp

This comment has been minimized.

Owner

Bogdanp commented Apr 12, 2018

So this will block the log watcher thread from receiving signals, correct? What about the filesystem changes watcher then? I would assume that that needs a similar block set put in place.

@Bogdanp

This comment has been minimized.

Owner

Bogdanp commented Apr 12, 2018

Would a better approach be to block all signals before spawning the child threads and then unblock them in the main thread before setting up the signal handlers?

@2miksyn

This comment has been minimized.

Contributor

2miksyn commented Apr 12, 2018

Yes, this will block signal processing in log watcher thread. In this case signals always will be processed in main thread. Currently watch options not works. I did not study this problem. It may be problem of watchdog module. In this case this problem be hard - watchdog module has not been updated for 3 years.

@2miksyn

This comment has been minimized.

Contributor

2miksyn commented Apr 12, 2018

Your approach will work too, but, think, this is not good practice - code will become harder. And I did not see any advantages - signals will be passed at start time too.

Bogdanp added a commit that referenced this pull request Apr 12, 2018

fix(signals): block signals in the master process' threads
This ought to fix issues with signal handling on older versions of
FreeBSD. See #66.
@Bogdanp

This comment has been minimized.

Owner

Bogdanp commented Apr 12, 2018

The problem is the main thread can spawn many threads and I don't want to litter each thread with signal blocking code. I believe the blocking-then-unblocking approach is actually better from a code quality perspective.

Please try this branch out and let me know if it works for you: https://github.com/Bogdanp/dramatiq/tree/signal-blocking

@Bogdanp

This comment has been minimized.

Owner

Bogdanp commented Apr 14, 2018

I ended up merging that branch so please try master instead.

@Bogdanp Bogdanp closed this Apr 14, 2018

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