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

Windows compatibility #119

Closed
wants to merge 39 commits into from
Closed

Windows compatibility #119

wants to merge 39 commits into from

Conversation

ryansm1
Copy link
Contributor

@ryansm1 ryansm1 commented Oct 9, 2018

Summary of changes:

  • Move code from __main__.py into main.py to prevent picklability errors on Windows when spawning new processes (diff main.py with __main__.py to see line-by-line edits)
  • Use multiprocessing's Process to spawn worker processes instead of os.fork()
  • Remove TimeLimit middleware on Windows platforms
  • Use logging's QueueHandler instead of pipes for interprocess logging
  • Setup SIGHUP (POSIX) and SIGBREAK (Windows) signal handlers only if they are available
  • Skip tests that cannot pass on Windows (e.g. tests requiring signals)

Known issues:

  • Gevent script is broken on Windows

@Bogdanp
Copy link
Owner

Bogdanp commented Oct 16, 2018

Thanks for doing this! I didn't get a chance to look at these changes this weekend, but I'll try to do it this upcoming one.

@ryansm1
Copy link
Contributor Author

ryansm1 commented Oct 20, 2018

Hopefully the additional commits don't interrupt your review; I wanted to clean up a couple of things that I missed after my last round of changes. It looks like some of the RabbitMQ retry-after-delay tests fail intermittently.

These are pretty substantial modifications to the main script so I expect them to need some additional work; let me know how I can help!

@Bogdanp
Copy link
Owner

Bogdanp commented Oct 21, 2018

Thanks again for doing this! I've reviewed your changes and made some of my own on top in #126 . Please take a look and let me know what you think.

I'm going to sit on that PR for a bit because it breaks gevent support (as mentioned in one of your commits) and disabling thread patching is not acceptable since the reason you're likely to run gevent with dramatiq is because you want to run many worker threads concurrently. This PR seems like it may fix the issue but there's no real indication of when it's going to be ready so we might have to find a different solution in the mean time (maybe multiprocessing.Pipes?).

@Bogdanp Bogdanp closed this Oct 21, 2018
@ryansm1
Copy link
Contributor Author

ryansm1 commented Oct 23, 2018

I originally converted the logging worker using multiprocessing.Pipes (see removal in #c890dfc) but dropped it in favor of the queue method because there is a native QueueHandler and the code was considerably uglier.

It's easy enough for me to put this back in if you have a good Gevent-based workflow to test against. Assuming it works without omitting the threading monkeypatching, we can explore some way of cleaning the code up (subclassing a base handler?).

I'll send another PR later this week and we can see how you'd like to proceed.

@Bogdanp
Copy link
Owner

Bogdanp commented Oct 24, 2018

It's easy enough for me to put this back in if you have a good Gevent-based workflow to test against. Assuming it works without omitting the threading monkeypatching, we can explore some way of cleaning the code up (subclassing a base handler?).

Yes, let's try that. I have a couple large applications I could test it on. Also, one thing that's regressed with this change is print calls (i.e. writing to stdout and stderr) are no longer serialized. In master, I redirect each worker process' stdout and stderr to the logging pipe. Perhaps with pipes we can bring that back.

@p1g3 p1g3 mentioned this pull request Feb 21, 2021
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