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

RFC: Always run workers in a subprocess. #2364

Closed
ahopkins opened this issue Jan 10, 2022 · 8 comments · Fixed by #2499
Closed

RFC: Always run workers in a subprocess. #2364

ahopkins opened this issue Jan 10, 2022 · 8 comments · Fixed by #2499
Labels

Comments

@ahopkins
Copy link
Member

Proposal: Always run workers in a subprocess.

Currently, when in PRODUCTION mode with workers=1, the server runs in the main process. This can lead to a discrepancy since the server(s) will be in a subprocess if:

  • there are multiple workers
  • auto_reload is enabled

We can create a more consistent experience if it will always be in a subprocess.

@ahopkins ahopkins transferred this issue from sanic-org/sanic-ext Jan 10, 2022
@ahopkins ahopkins added the RFC label Jan 10, 2022
@ahopkins ahopkins changed the title Proposal [RFC] Always run workers in a subprocess. Jan 10, 2022
@ahopkins ahopkins changed the title [RFC] Always run workers in a subprocess. RFC: Always run workers in a subprocess. Jan 10, 2022
@Tronic
Copy link
Member

Tronic commented Jan 12, 2022

I considered this in the Trio fork I worked on, and made workers=0 mean that it runs in the main process, while workers=1 spawns a subprocess for the single worker. There are good reasons to run a server without any subprocesses, especially in debug mode, but also in production, so running without subprocesses should remain an option at least.

If this causes trouble in multi-serve or other development, the main Sanic.serve() could become always subprocess-using if there was a await Sanic.serve_async() style lower level API for those who need strictly single process (or ability to run other things on the same asyncio loop, possibly deprecating the always discouraged create server method of doing that, if it can be better supported with a new API).

@ahopkins
Copy link
Member Author

You make a good point. I think the core of what I was going after is the "gotcha" of a newcomer. This can easily be added with the new Sanic.serve style once that is merged.

I am currently leaning towards workers=0 since that means no additional API, and no competing config values (example workers=2, single_process=True.

He big change is really documentation then.

@Tronic
Copy link
Member

Tronic commented Jan 12, 2022

I don't remember the exact state of CLI flags in 22.3, but it would probably then make sense to have --debug imply workers=0 by default, while --dev sticks to workers=1 to avoid surprises when switching to production mode. I recall PyCharm also had some particular issue with subprocesses, and pytest/coverage is certainly a bit flaky with them.

@ahopkins
Copy link
Member Author

That I think would cause more confusion. Currently:

--dev == --debug + --auto_reload 

For debug mode to then change one thing would be non-obvious.

@Tronic
Copy link
Member

Tronic commented Jan 12, 2022

I am leaning more and more towards not spawning any workers unless using --fast. But probably some other developers/users should weigh in. I always run with multiple workers anyway, and have little more to add to this.

@ahopkins
Copy link
Member Author

Okay, so I have put some more thoughts and effort into this. I think that we should perform the following:

  • Sanic.serve (and therefore also the sanic CLI) will always run a main process, with and use multiprocessing for starting one or more workers in a subprocess
  • Doing this will allow us to also open up an API for passing things like a multiprocessing.Queue among multiple workers (on the same interpreter of course)
  • Sanic.serve_single will be for running serve in a single process
  • Autoreload needs an overhaul as well - rather than running the server with subprocess.Popen, I think we should also do this with multiprocessing. This means autoreload would not be possible from serve_single
  • Sanic.prepare should get a new flag so that you can prepare multiple applications to run separately in their own processes instead of sharing (this will be helpful in the HTTP/3 solution)

@Tronic
Copy link
Member

Tronic commented Mar 23, 2022

All that sounds good. Any chance of having a pathway to fixing Windows (or MacOS) where forking is not possible?

@ahopkins
Copy link
Member Author

All that sounds good. Any chance of having a pathway to fixing Windows (or MacOS) where forking is not possible?

Yes, I think this is doable. I have had it working, but not done a full test yet to make sure it does not cause a regression somewhere else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants