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

Add cli flag to start worker processes with daemon=False #289

Closed
wants to merge 1 commit into from

Conversation

takhs91
Copy link
Contributor

@takhs91 takhs91 commented Mar 23, 2020

This allows the use of multiprocessing.Pool inside an actor as discussed in https://www.reddit.com/r/dramatiq/comments/fdus4w/support_for_cpu_intensive_workloads_and/

I initially ran the tests with daemon hardcoded to False and they passed but I made a cli option to reduce any chances altering the current functionality. I furthemore tried to analyze what changes it could create

Since you have already written sighandlers and logic about waiting the worker processes to exit using daemon=False should't leave any orphaned workers assuming cli executes normally and gets shutdowned by signals other than sigkill or sigquit(those also leaves the worker processes orphaned even with daemon=True it could probably only be solved from their side).

If an exception happens in cli before or inside the current process waiting code, it seem to be causing a deadlock in exiting in either way the daemon flag is set.

To conclude if you want to enable actors using multiprocessing I believe that this will cause no harm to existing functionality with either option set. Solving any of the afformentioned issues should probably be tackled in different issues. I would like to hear your thoughts!

@takhs91 takhs91 marked this pull request as ready for review Mar 27, 2020
@Bogdanp
Copy link
Owner

@Bogdanp Bogdanp commented Mar 28, 2020

Thanks! Let's go the other route: might as well not add a new flag unless we need to. I'll go through some of my own test cases once you make the change.

@Bogdanp
Copy link
Owner

@Bogdanp Bogdanp commented Mar 28, 2020

Since I closed the other PR, please add yourself to the contributors file here. Thanks again!

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

2 participants