bpo-35934: Add socket.create_server() utility function #11784
Merged
Conversation
This file contains 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
…REUSEPORT is not supported
giampaolo
requested review from
1st1,
asvetlov,
rhettinger and
terryjreedy
as code owners
asvetlov
reviewed
Feb 7, 2019
vadmium
reviewed
Feb 11, 2019
Hum, the constant is used in the following example: Do we miss some header files? |
|
Yes it appears we miss some constants on Windows. That is being tracked here: |
vadmium
reviewed
Mar 6, 2019
Doc/library/socket.rst
Outdated
| @@ -595,6 +595,53 @@ The following functions all create :ref:`socket objects <socket-objects>`. | |||
| .. versionchanged:: 3.2 | |||
| *source_address* was added. | |||
|
|
|||
| .. 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 | |||
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` |
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) |
There was a problem hiding this comment.
Is type a valid keyword? If not, maybe this needs a test case :)
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: |
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)
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) |
There was a problem hiding this comment.
Is this event necessary? It is set just after the thread is started.
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.
|
@giampaolo: Sorry, I don't have the bandwidth right now to review this PR. |
|
@vstinner it's ok, we still depend on https://bugs.python.org/issue29515 which should be fixed first. |
|
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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