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

multiprocessing.cpu_count() is dangerous in containers #2028

Closed
twosigmajab opened this issue May 2, 2019 · 4 comments
Closed

multiprocessing.cpu_count() is dangerous in containers #2028

twosigmajab opened this issue May 2, 2019 · 4 comments

Comments

@twosigmajab
Copy link

workers = multiprocessing.cpu_count() * 2 + 1

Due to https://bugs.python.org/issue36054, the recommendation in these docs is dangerous when running inside a container. If your container has been allocated (say) a single core and 512 MB of RAM on a machine with 64 cores, multiprocessing.cpu_count() thinks it has 64 cores instead of just 1, and you end up spawning 129 workers. This is way more than the container has resources for, and ends up causing out of memory crashes.

@tilgovi
Copy link
Collaborator

tilgovi commented May 2, 2019

Hello 😄! Thanks for the notification.

The default in Gunicorn is a single worker, even if the documentation has an example of detecting the number. Given that this looks likely to be a Python bug fix eventually, I'm not sure there's anything we should do in particular here. Filing it downstream at flask-gunicorn makes a lot of sense, though.

@benoitc
Copy link
Owner

benoitc commented May 23, 2019

it's more something that should be fixed in python not really an issue there. The documentation has been made at the time docker were not even existing but I do think it's still useful for the example since it's very explicit. Closing the issue then.

@benoitc benoitc closed this as completed May 23, 2019
@twosigmajab
Copy link
Author

Adding 1 tiny line to the docs to say ("but beware of BPO-36054 if using containers!") could save many users headaches for the ~years they'll still be using versions of Python that don't have this fixed. If you're not opposed to merging that, I'll submit a PR if no one beats me to it.

@benoitc
Copy link
Owner

benoitc commented May 23, 2019

@twosigmajab good idea! please go ahead :)

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

No branches or pull requests

3 participants