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

Allow passing of sockets for socket activation #215

Conversation

Frank-Krick
Copy link
Contributor

@Frank-Krick Frank-Krick commented Oct 29, 2018

As discussed in issue #204, the pull request adds the new parameter sockets to serve and create_server which can be used to create waitress instances using already existing and bound sockets, e.g. for socket activation with systemd or launchd.

The changes are tested with several additional test cases.

In addition to the code changes, it adds a new page to the documentation and a new entry to describe the new parameter sockets in the page for arguments. Adding the entry in arguments is done under the assumption that the next version will be 1.1.1 to supply the version number for versionadded.

The changes only add the parameter, they do not handle init system specific tasks like creating sockets from file descriptors because we want to avoid dependencies to init systems.

Closes #204


This change is Reviewable

@stevepiercy
Copy link
Member

@Frank-Krick there's one line that lacks coverage. Please fix that.

@stevepiercy
Copy link
Member

For maintainers, I noticed this statement:
https://travis-ci.org/Pylons/waitress/jobs/447585698#L484

/home/travis/build/Pylons/waitress/.tox/py2-cover/local/lib/python2.7/site-packages/pip/_vendor/urllib3/util/ssl_.py:160: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/advanced-usage.html#ssl-warnings

Should I make a new issue for that?

@stevepiercy
Copy link
Member

@Frank-Krick Please sign CONTRIBUTORS.txt and push that commit to this PR.

Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

My change requests are concern docs and error messages.

Make sure you sign CONTRIBUTORS.txt, too.

docs/arguments.rst Outdated Show resolved Hide resolved
docs/socket-activation.rst Outdated Show resolved Hide resolved
docs/socket-activation.rst Outdated Show resolved Hide resolved
docs/socket-activation.rst Outdated Show resolved Hide resolved
waitress/adjustments.py Outdated Show resolved Hide resolved
docs/arguments.rst Outdated Show resolved Hide resolved
@Frank-Krick
Copy link
Contributor Author

Hi @stevepiercy, I'm done with the requested changes.

Btw. in adjustments.py, line 231, there is still the "host and or port" in the error message related to listen and host, port. Let me know if I should change that in this pull request too, for the sake of consistency.

Thanks,

Frank

@stevepiercy
Copy link
Member

Btw. in adjustments.py, line 231, there is still the "host and or port" in the error message related to listen and host, port. Let me know if I should change that in this pull request too, for the sake of consistency.

Yes, please! Thank you!

@Frank-Krick
Copy link
Contributor Author

Done, thanks.

Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Thank you! The core maintainers will review when they are available.

@digitalresistor
Copy link
Member

Thanks very much for this work! I'm currently under the weather with a cold, but will get to this asap to review it and give you feedback and or accept it!

@Frank-Krick
Copy link
Contributor Author

You're welcome and get well soon.

docs/socket-activation.rst Outdated Show resolved Hide resolved
docs/socket-activation.rst Outdated Show resolved Hide resolved
waitress/server.py Outdated Show resolved Hide resolved
@digitalresistor
Copy link
Member

If you could roll #217 into this as well, that would be great!

@Frank-Krick
Copy link
Contributor Author

I'll do the changes to the documentation and roll in #217. Please have a look at my comment above regarding the use of different socket types (UNIX vs INET).

@digitalresistor
Copy link
Member

@stevepiercy:

For maintainers, I noticed this statement:
https://travis-ci.org/Pylons/waitress/jobs/447585698#L484

/home/travis/build/Pylons/waitress/.tox/py2-cover/local/lib/python2.7/site-packages/pip/_vendor/urllib3/util/ssl_.py:160: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/advanced-usage.html#ssl-warnings

Should I make a new issue for that?

No. It's not something I care to fix.

@Frank-Krick
Copy link
Contributor Author

Hi @bertjwregeer,

I added the check to prevent mixing of UNIX and INET sockets. The same check also throws an error if the socket has a type other than Stream or a family other than INET, INET6 or UNIX.

This should address all your review comments.

Thanks,

Frank

@digitalresistor
Copy link
Member

Frank, thank you very much!

Last but not least, we need some defensive coding in check_sockets because the tests currently fail on Windows: https://ci.appveyor.com/project/Pylons/waitress/builds/20276586/job/vwpcsnou3hhgijmm

@Frank-Krick
Copy link
Contributor Author

I added a check for using hasattr(socket, 'AF_UNIX') to make the test case work on Windows. That means that there will be a line of code not covered using the unit tests on Windows. Will that create problems during the test runs?

@digitalresistor
Copy link
Member

No, we don't check coverage on Windows, only on MacOS (where I develop) and Linux (Where Travis runs).

Looks like we are good now. I'll give it one last once over tomorrow and then get it merged :-)!

Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Couple of minor docs change requests.

docs/socket-activation.rst Show resolved Hide resolved
docs/socket-activation.rst Outdated Show resolved Hide resolved
@Frank-Krick
Copy link
Contributor Author

@stevepiercy I've done the changes. Thanks

@digitalresistor digitalresistor merged commit ecea02f into Pylons:master Nov 14, 2018
@digitalresistor
Copy link
Member

@Frank-Krick thanks very much!

from init systems, process and socket managers, or other launchers that can provide
pre-bound sockets.

The following shows a code example starting waitress with two Internet sockets pre-bound sockets.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence does not make sense. I hoped that merging the two lines into one would have been cleaned up by removing the extra "sockets". I'll submit a quick PR to fix it.

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

3 participants