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 http-fast-listen option for waitress #75

Merged
merged 9 commits into from
Mar 7, 2019
Merged

Conversation

tschorr
Copy link
Contributor

@tschorr tschorr commented Feb 28, 2019

s. #71 (and also zopefoundation/Zope#416).

waitress.serve() accepts a list of (prebound) sockets, https://waitress.readthedocs.io/en/latest/arguments.html.

Suggestion is to bind a socket before loading the application and to use a custom server_factory to make this work with PasteDeploy. This could also be done in https://github.com/zopefoundation/Zope, but plone.recipe.zope2instance is already relying on waitress anyway (Zope doesn't really seem to depend on waitress even though it is currently required it in it's setup.py).

@mister-roboto
Copy link

@tschorr thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@tschorr
Copy link
Contributor Author

tschorr commented Feb 28, 2019

@jenkins-plone-org please run jobs

1 similar comment
@tschorr
Copy link
Contributor Author

tschorr commented Feb 28, 2019

@jenkins-plone-org please run jobs

@tschorr tschorr changed the title WIP: add http-fast-listen option for waitress Add http-fast-listen option for waitress Mar 1, 2019
@tschorr tschorr requested review from davisagli, icemac and jensens March 1, 2019 08:55
@tschorr
Copy link
Contributor Author

tschorr commented Mar 1, 2019

@jenkins-plone-org please run jobs

Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

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

So, I am not an expert exactly on WSGI and waitress, but it looks ok. Was it tested on Windows?

@icemac
Copy link

icemac commented Mar 5, 2019

Hm, there are broken tests, I'd like to review after they are fixed.

@tschorr
Copy link
Contributor Author

tschorr commented Mar 5, 2019

There are broken doctests after merging master. I'm about to fix them. Don't merge

…deps, coevered so far by buildout version pins)
@tschorr
Copy link
Contributor Author

tschorr commented Mar 5, 2019

@jenkins-plone-org please run jobs

@tschorr
Copy link
Contributor Author

tschorr commented Mar 5, 2019

@icemac Travis tests are green now and the one failing Robot Test on Jenkins looks unrelated. Feel free to review.

@jensens I currently don't have an opportunity to test this on Windows

Copy link

@icemac icemac left a comment

Choose a reason for hiding this comment

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

LGTM. I only read the code, I did not try it out.

I got confused a bit by some function names, maybe they could be changed for clarity.

src/plone/recipe/zope2instance/ctl.py Outdated Show resolved Hide resolved
src/plone/recipe/zope2instance/ctl.py Show resolved Hide resolved
@tschorr
Copy link
Contributor Author

tschorr commented Mar 6, 2019

@icemac I'm basically following https://pastedeploy.readthedocs.io/en/latest/#paste-server-factory. I opted for import waitress and waitress.serve to avaoid the ambiguity.

@icemac
Copy link

icemac commented Mar 7, 2019

@jenkins-plone-org please run jobs

@tschorr
Copy link
Contributor Author

tschorr commented Mar 7, 2019

Again the one failing robot test looks unrelated.

@jensens
Copy link
Member

jensens commented Mar 7, 2019

Indeed, I merge this one now.

@jensens jensens merged commit a650a72 into master Mar 7, 2019
@jensens jensens deleted the http-fast-listen-wsgi branch March 7, 2019 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants