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

switch waitress for gunicorn #98

Merged
merged 3 commits into from
Oct 29, 2020
Merged

switch waitress for gunicorn #98

merged 3 commits into from
Oct 29, 2020

Conversation

fmigneault
Copy link
Contributor

Fixes #97

Replaces waitress by gunicorn by default.
waitress seems to do some kind of deduplication of contents with extra read/write of chucks during data streaming.
I could not find any way to avoid waitress to do this with some option/flag.

Tests results can be found here:
#97 (comment)
#97 (comment)

I leave waitress in dependencies so that pre-existing deploys can use whichever they want between gunicorn / waitress.
(if this is not an issue, we can remove it also)

Important note:
gunicorn > 20 needs to be started with pserve to have correct port assigned.
There is a change of parsing PasteDeploy config where starting directly with gunicorn doesn't detect the right port.
(doesn't matter that much in this case, since default is 8000 which is also the one employed)

Regarless, Docker image was already using pserve, so everything should work just fine.

@fmigneault fmigneault requested review from cehbrecht and tlvu October 29, 2020 16:05
Copy link

@tlvu tlvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment. LGTM.

development.ini Show resolved Hide resolved
@tlvu
Copy link

tlvu commented Oct 29, 2020

By the way, that's a very clear PR describing the changes. I would suggest you copy/paste this description into the merge commit description to preserve it and make it available even if github is not available (ex: no network).

@cehbrecht
Copy link
Member

I leave waitress in dependencies so that pre-existing deploys can use whichever they want between gunicorn / waitress.
(if this is not an issue, we can remove it also)

@fmigneault I think you can remove waitress.

@cehbrecht
Copy link
Member

@fmigneault Thanks for the PR :) Feel free to merge.

Fixes #97

Replaces `waitress` by `gunicorn` by default.
`waitress` seems to do some kind of deduplication of contents with extra read/write of chucks during data streaming.
Could not find any way to avoid `waitress` to do this with some option/flag.

Tests results can be found here:
- #97 (comment)
- #97 (comment)

Leaving waitress in dependencies so that pre-existing deploys can use whichever they want between `gunicorn` / `waitress`.

Important note:
`gunicorn > 20` needs to be started with `pserve` to have correct port assigned.
There is a change of parsing `PasteDeploy` config where starting directly with `gunicorn` doesn't detect the right port.
(doesn't matter that much in this case, since default is 8000 which is also the one employed)

Regarless, Docker image was already using `pserve`, so everything should work just fine.
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.

3 participants