bpo-35934: Add socket.create_server() utility function#11784
Merged
bpo-35934: Add socket.create_server() utility function#11784
Conversation
…REUSEPORT is not supported
giampaolo
requested review from
1st1,
asvetlov,
rhettinger and
terryjreedy
as code owners
asvetlov
reviewed
Feb 7, 2019
Contributor
asvetlov
left a comment
asvetlov
left a comment
There was a problem hiding this comment.
I like the proposal but have a few comments regarding implementation.
giampaolo
commented
Feb 8, 2019
Contributor
Author
giampaolo
left a comment
giampaolo
left a comment
There was a problem hiding this comment.
added reuse_addr parameter
vadmium
reviewed
Feb 11, 2019
Contributor
Author
|
Yes it appears we miss some constants on Windows. That is being tracked here: |
vadmium
reviewed
Mar 6, 2019
Doc/library/socket.rst
Outdated
|
|
||
| .. function:: create_server(address, *, family=AF_INET, backlog=0, reuse_port=False, dualstack_ipv6=False) | ||
|
|
||
| Convenience function which creates a :data:`SOCK_STREAM` type socket |
Member
There was a problem hiding this comment.
Any reason why you don't just say it creates a TCP socket? I think this is guaranteed by Posix for AF_INET and AF_INET6 with SOCK_STREAM and proto set to zero.
Doc/library/socket.rst
Outdated
| .. function:: has_dualstack_ipv6() | ||
|
|
||
| Return ``True`` if the platform supports creating a :data:`SOCK_STREAM` | ||
| socket which can handle both :data:`AF_INET` or :data:`AF_INET6` |
Member
There was a problem hiding this comment.
I think AF_INET and AF_INET6 are mutually exclusive for a given socket. Perhaps it is more accurate to say the socket can handle both IP v4 and v6.
Lib/idlelib/rpc.py
Outdated
| self.listening_sock.bind(address) | ||
| self.listening_sock.listen(1) | ||
| self.listening_sock = socket.create_server( | ||
| address, family=family, type=type, backlog=1) |
Member
There was a problem hiding this comment.
Is type a valid keyword? If not, maybe this needs a test case :)
Contributor
Author
There was a problem hiding this comment.
Ouch! I missed that, thanks. I just removed this part and left idlelib alone in f786884. I will probably get back at this in a separate PR because it's more controversial.
Lib/socket.py
Outdated
| raise error(err.errno, msg) from None | ||
| sock.listen(backlog) | ||
| return sock | ||
| except Exception: |
Member
There was a problem hiding this comment.
Use plain except to catch all exceptions (e.g. KeyboardInterrupt could be raised during a slow DNS lookup)
Contributor
Author
There was a problem hiding this comment.
Good point. I think it's saner to just catch socket.error, the same way it's done in create_connection(). Changed it in f786884.
| with sock: | ||
| conn, _ = sock.accept() | ||
| with conn: | ||
| event.wait(self.timeout) |
Member
There was a problem hiding this comment.
Is this event necessary? It is set just after the thread is started.
Contributor
Author
There was a problem hiding this comment.
Yes it is because you want to wait for accept() to succeed before doing connect(). Without this sync primitive in place the test will fail.
Member
|
@giampaolo: Sorry, I don't have the bandwidth right now to review this PR. |
Contributor
Author
|
@vstinner it's ok, we still depend on https://bugs.python.org/issue29515 which should be fixed first. |
Contributor
Author
|
OK, I just committed a fix for https://bugs.python.org/issue29515 which adds IPPROTO_IPV6 constant on Windows. Since dual-stack is supported on Windows I removed the reference to the external recipe from the doc (at this point I am not aware of any platform NOT supporting dual-stack). I think this is good to go but I will wait one more week before merging just in case somebody else wants to comment. Thanks everybody for the great review and suggestions without which this couldn't have happened. |
|
|
|
giampaolo
added a commit
that referenced
this pull request
Apr 9, 2019
It turns out doing socket.listen(0) does not equal to "choose a reasonable default". It actually means "set backlog to 0". As such set backlog=None as the default for socket.create_server. Fixes the following BB failures: #11784 (comment) Ref. BPO-1756, GH-11784.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://bugs.python.org/issue35934