Issue 18643: add a fallback socketpair() implementation to the socket module (original) (raw)
Created on 2013-08-03 12:11 by neologix, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (28)
Author: Charles-François Natali (neologix) * 
Date: 2013-08-03 12:11
socketpair() is quite useful, notably for tests. Currently, it's not defined on Windows. Since it's rather easy to implement, it would be nice to have it, if not in the stdlib, at least in test.support.
Author: Charles-François Natali (neologix) *
Date: 2013-08-16 10:48
I was thinking about dropping the C wrapper from socketmodule.c, and replacing it with a pure Python implementation (e.g. the one posted by Richard on python-dev). What do you think?
Author: Richard Oudkerk (sbt) *
Date: 2013-08-16 15:52
Do you mean you want to use a pure python implementation on Unix?
Then you would have to deal with AF_UNIX (which is the default family for socketpair() currently). A pure python implementation which deals with AF_UNIX would have to temporarily create a listening socket on the file system and deal with races like tempfile.mkstemp() does. I think it is simpler to only use the pure python implementation on Windows.
BTW, it is possible for another process to connect to the address we temporily bind to. Neither my version nor the one in tulip handle that correctly.
Author: Charles-François Natali (neologix) *
Date: 2013-08-24 08:37
Here's a patch.
Note that I'm still not sure whether it belong to the socket module or test.support.
Author: STINNER Victor (vstinner) *
Date: 2013-08-24 10:16
On Linux, many tests of test_socket are failing with the pure Python implementation.
Author: Charles-François Natali (neologix) *
Date: 2013-08-24 10:44
On Linux, many tests of test_socket are failing with the pure Python implementation.
The only failing tests I see are due to the fact that the Python implementation uses AF_INET instead of AF_UNIX, which is normal since Windows doesn't have AF_UNIX. Do you see other failures?
Author: STINNER Victor (vstinner) *
Date: 2013-08-24 13:52
Do you see other failures?
I applied your patch and I just replace the hasattr with: hasattr(_socket, "Xsocketpair"). test_socket failures:
====================================================================== FAIL: test_sendall_interrupted (test.test_socket.GeneralModuleTests)
Traceback (most recent call last): File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 1259, in test_sendall_interrupted self.check_sendall_interrupted(False) File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 1248, in check_sendall_interrupted c.sendall(b"x" * (1024**2)) AssertionError: ZeroDivisionError not raised
====================================================================== FAIL: test_sendall_interrupted_with_timeout (test.test_socket.GeneralModuleTests)
Traceback (most recent call last): File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 1262, in test_sendall_interrupted_with_timeout self.check_sendall_interrupted(True) File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 1248, in check_sendall_interrupted c.sendall(b"x" * (1024**2)) AssertionError: ZeroDivisionError not raised
====================================================================== FAIL: testDefaults (test.test_socket.BasicSocketPairTest)
Traceback (most recent call last): File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 3640, in testDefaults self._check_defaults(self.serv) File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 3630, in _check_defaults self.assertEqual(sock.family, socket.AF_UNIX) AssertionError: 2 != 1
====================================================================== FAIL: testDefaults (test.test_socket.BasicSocketPairTest)
Traceback (most recent call last): File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 261, in _tearDown raise exc File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 273, in clientRun test_func() File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 3637, in _testDefaults self._check_defaults(self.cli) File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 3630, in _check_defaults self.assertEqual(sock.family, socket.AF_UNIX) AssertionError: 2 != 1
Author: Charles-François Natali (neologix) *
Date: 2013-08-24 22:42
self.assertEqual(sock.family, socket.AF_UNIX)AssertionError: 2 != 1
This is normal.
======================================================================
FAIL: test_sendall_interrupted (test.test_socket.GeneralModuleTests)
Traceback (most recent call last): File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 1259, in test_sendall_interrupted self.check_sendall_interrupted(False) File "/home/haypo/prog/python/default/Lib/test/test_socket.py", line 1248, in check_sendall_interrupted c.sendall(b"x" * (1024**2)) AssertionError: ZeroDivisionError not raised
Not this one. I can't reproduce it on my Linux box, could you post the result of:
$ strace -ttTf ./python -m test -v -m test_sendall_interrupted test_socket
Author: Charles-François Natali (neologix) *
Date: 2013-08-25 10:10
Alright, I think I know what's happening.
The Python implementation uses a TCP socket, whereas the native implementation uses AF_UNIX socket. The maximum size of data that can be written to a socket without blocking is given by its send/receive buffers. On Linux, the default buffer sizes are set by: net.core.(r|w)mem_default
but for TCP sockets, its set by: net.ipv4.tcp_(r|w)mem
So on your machine, you probably have tcp_(r|w)mem quite larger than (r|w)mem, so the sendall test doesn't write enough data to the socket to block.
The solution is simply to increase the amount of data written. Could you try the attached patch?
If it works I'll commit it, because the test isn't really reliable (i.e. it could fail on a machine with a large (r|w)mem_default).
Author: Charles-François Natali (neologix) *
Date: 2013-08-28 18:27
Victor, did you have a chance to test the patch?
Author: STINNER Victor (vstinner) *
Date: 2013-08-28 19:50
Victor, did you have a chance to test the patch?
I tested socketpair.diff (modified to use it even on Linux) on another computer, and test ran successfully.
On my main computer (Fedora 18, Linux kernel 3.9.4), the test is failing. With sendall_write.diff, the test does pass. test.support.PIPE_MAX_SIZE=4194305 (4 MB) on this computer. I don't know for the other one.
Author: Charles-François Natali (neologix) *
Date: 2013-08-28 20:54
Since I'll update socket tests to also use support.PIPE_MAX_SIZE, maybe we should rename this variable while we're at it? Anybody can think of a name that would work both for sockets and pipes?
Author: STINNER Victor (vstinner) *
Date: 2013-08-28 20:56
"Since I'll update socket tests to also use support.PIPE_MAX_SIZE, maybe we should rename this variable while we're at it?"
Why not using a different value (constant) for sockets?
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-28 21:01
By alternating between PIPE and SOCK, you get POPK, which is as descriptive as it gets.
Author: Charles-François Natali (neologix) *
Date: 2013-08-28 21:29
Why not using a different value (constant) for sockets?
We can add a new constant (e.g. SOCK_MAX_SIZE), that would be fine, as long as it aliases to PIPE_MAX_SIZE. There's no need to have two values (the current 4MB value is fine).
By alternating between PIPE and SOCK, you get POPK, which is as descriptive as it gets.
Thank you Antoine :-)
I'll think I'll go for: SOCK_MAX_SIZE = PIPE_MAX_SIZE = 4MB
Author: Roundup Robot (python-dev) 
Date: 2013-08-29 17:27
New changeset 2de7fc69d2d4 by Charles-François Natali in branch '2.7': Issue #18643: Fix some test_socket failures due to large default socket buffer http://hg.python.org/cpython/rev/2de7fc69d2d4
New changeset 498957c97c2b by Charles-François Natali in branch '3.3': Issue #18643: Fix some test_socket failures due to large default socket buffer http://hg.python.org/cpython/rev/498957c97c2b
New changeset 9b27cf72c79b by Charles-François Natali in branch 'default': Issue #18643: Fix some test_socket failures due to large default socket buffer http://hg.python.org/cpython/rev/9b27cf72c79b
Author: Charles-François Natali (neologix) *
Date: 2013-08-29 18:24
Alright, I chose a 16MB size for SOCK_MAX_SIZE because nowadays, one can encounter large buffers for Gigabit/10Gb Ethernet.
Regarding the original issue: is it OK to add it as socket.socketpair(), or should it only be added to test.support? Since I don't use Windows, I don't know if this deserves being expose in socket module, but having socketpair() available in the test suite would be really useful (for example, most select tests aren't run on Windows because they use pipes).
Author: Charles-François Natali (neologix) *
Date: 2013-11-30 15:39
Here's a patch adding socketpair to test.support.
This version has been used in test_selectors for quite some time now, and would probably be useful for other tests as well.
Author: Charles-François Natali (neologix) *
Date: 2014-07-23 20:14
Barring any objection, I'll commit the patch attached within a couple days.
Author: STINNER Victor (vstinner) *
Date: 2014-07-23 20:41
Barring any objection, I'll commit the patch attached within a couple days.
I have objections!
You should copy the code from asyncio.windows_utils.socketpair(). It checks also type and proto parameter: raise a ValueError for unsupported values (only SOCK_STREAM and proto=0 are supported). It also supports IPv6. It handles BlockingIOError and InterruptedError on connect (I never understood why these exceptions are simply ignored).
You patch checks the address received by accept() and retry. It's a little bit surprising. If you get an unexpected connection, maybe an exception should be raised, instead of just ignoring it. Maybe we should modify the code in asyncio to check also the address?
Finally, please add socketpair() directly to the socket module. It is useful for applications, not only for tests.
A socket.socketpair() function available on Windows would help me to write tests for the issue #22018!
Author: Charles-François Natali (neologix) *
Date: 2014-07-23 22:21
You should copy the code from asyncio.windows_utils.socketpair(). It checks also type and proto parameter: raise a ValueError for unsupported values (only SOCK_STREAM and proto=0 are supported). It also supports IPv6. It handles BlockingIOError and InterruptedError on connect (I never understood why these exceptions are simply ignored).
I could use it then, although all those checks are unnecessary since the underlying socket() call will check it for us.
You patch checks the address received by accept() and retry. It's a little bit surprising. If you get an unexpected connection, maybe an exception should be raised, instead of just ignoring it. Maybe we should modify the code in asyncio to check also the address?
Why? If you have an unexpected connection, it should be dropped, and we should wait until the client we're expecting connects, there's no reason to raise an error. But I'll just reuse asyncio's version.
Finally, please add socketpair() directly to the socket module. It is useful for applications, not only for tests.
Well, that's what I suggested initially, but never got any reply, so I went for the least intrusive. I personally think too it should belong to socket module.
So here it is.
Author: STINNER Victor (vstinner) *
Date: 2014-07-23 23:34
2014-07-24 0:21 GMT+02:00 Charles-François Natali <report@bugs.python.org>:
You should copy the code from asyncio.windows_utils.socketpair(). It checks also type and proto parameter: raise a ValueError for unsupported values (only SOCK_STREAM and proto=0 are supported). It also supports IPv6. It handles BlockingIOError and InterruptedError on connect (I never understood why these exceptions are simply ignored).
I could use it then, although all those checks are unnecessary since the underlying socket() call will check it for us.
socket.socket() supports SOCK_DGRAM on Windows, but asyncio socketpair() function does not. The check on the socket family is just to raise a better error message than an error on connect() or something else.
I don't remember why I added a specific check on the proto parameter.
We might support more protocols in the future, but I'm not sure that it's interesting to support them (right now). I guess that most people will call socketpair() without any parameter.
You patch checks the address received by accept() and retry. It's a little bit surprising. If you get an unexpected connection, maybe an exception should be raised, instead of just ignoring it. Maybe we should modify the code in asyncio to check also the address?
Why? If you have an unexpected connection, it should be dropped, and we should wait until the client we're expecting connects, there's no reason to raise an error. But I'll just reuse asyncio's version.
Your version is safer. You should reuse your while dropping unknown connections.
By the way, we should reuse socket.socketpair() in asyncio.windows_utils. The Tulip is written for Python 3.3 and shares exactly the same code base, so you should write
Something like:
if hasattr(socket, 'socketpair'): socketpair = socket.socketpair else: def socketpair(...): ...
Please also fix socketpair() in asyncio to add the while/drop unknown connection (well, the function in socket.py and windows_utils.py must be the same).
Well, that's what I suggested initially, but never got any reply, so I went for the least intrusive. I personally think too it should belong to socket module.
On Windows, asyncio uses a socket pair in its default event loop (SelectorEventLoop) for its "self pipe", because select.select() only supports sockets. The Windows ProactorEventLoop also uses a socket pair for its "self pipe".
Oh, and you forgot to modify the documentation to update "Availability". Please add a ".. versionchanged:: 3.5" mentionning that the function is now also available on Windows.
Author: STINNER Victor (vstinner) *
Date: 2014-07-24 08:01
I don't remember why I added a specific check on the proto parameter.
I tested on Windows: socket.socket(proto=1) raises an OSError(WSAEPROTONOSUPPORT):
""" WSAEPROTONOSUPPORT 10043: Protocol not supported.
The requested protocol has not been configured into the system, or no implementation for it exists. For example, a socket call requests a SOCK_DGRAM socket, but specifies a stream protocol. """
Since the error comes directly at socket.socket(), we drop drop the explicit test in socketpair().
Author: Charles-François Natali (neologix) *
Date: 2014-07-24 08:11
By the way, we should reuse socket.socketpair() in asyncio.windows_utils. The Tulip is written for Python 3.3 and shares exactly the same code base, so you should write
Something like:
if hasattr(socket, 'socketpair'): socketpair = socket.socketpair else: def socketpair(...): ...
Please also fix socketpair() in asyncio to add the while/drop unknown connection (well, the function in socket.py and windows_utils.py must be the same).
That's a separate issue.
Oh, and you forgot to modify the documentation to update "Availability". Please add a ".. versionchanged:: 3.5" mentionning that the function is now also available on Windows.
Did you look at the patch?
363 .. versionchanged:: 3.5 364 Windows support added
Author: STINNER Victor (vstinner) *
Date: 2014-07-24 08:49
2014-07-24 10:11 GMT+02:00 Charles-François Natali <report@bugs.python.org>:
Please also fix socketpair() in asyncio to add the while/drop unknown connection (well, the function in socket.py and windows_utils.py must be the same).
That's a separate issue.
Ok.
Oh, and you forgot to modify the documentation to update "Availability". Please add a ".. versionchanged:: 3.5" mentionning that the function is now also available on Windows.
Did you look at the patch?
363 .. versionchanged:: 3.5 364 Windows support added
Ok, I missed this part.
In this case, socketpair-4.diff looks good to me. You can commit your patch in Python 3.5.
I will open another issue to synchronize asyncio, maybe fix accept() to check the address and drop the "if proto != 0:" test.
Author: STINNER Victor (vstinner) *
Date: 2014-10-11 14:22
In this case, socketpair-4.diff looks good to me. You can commit your patch in Python 3.5.
Hey Charles-François, can you commit your patch? I forgot that you did commited it yet, and I expected socket.socketpair() to be available on all platforms.
Author: Roundup Robot (python-dev)
Date: 2014-10-14 20:24
New changeset 6098141155f9 by Charles-François Natali in branch 'default': Issue #18643: Add socket.socketpair() on Windows. https://hg.python.org/cpython/rev/6098141155f9
Author: Roundup Robot (python-dev)
Date: 2014-10-14 21:08
New changeset 03d3f2664930 by Victor Stinner in branch '3.4': Issue #18643: asyncio.windows_utils now reuse socket.socketpair() on Windows if https://hg.python.org/cpython/rev/03d3f2664930
History
Date
User
Action
Args
2022-04-11 14:57:48
admin
set
github: 62843
2014-10-14 21:24:28
neologix
set
status: open -> closed
title: add a fallback socketpair() implementation in test.support -> add a fallback socketpair() implementation to the socket module
resolution: fixed
stage: patch review -> resolved
2014-10-14 21:08:07
python-dev
set
messages: +
2014-10-14 20:24:01
python-dev
set
messages: +
2014-10-11 14:22:36
vstinner
set
messages: +
2014-07-24 08:49:10
vstinner
set
messages: +
2014-07-24 08:11:52
neologix
set
messages: +
2014-07-24 08:01:44
vstinner
set
messages: +
2014-07-23 23:34:34
vstinner
set
messages: +
2014-07-23 22:21:34
neologix
set
files: + socketpair-4.diff
messages: +
2014-07-23 21:34:53
vstinner
set
versions: + Python 3.5, - Python 3.4
2014-07-23 20:41:50
vstinner
set
messages: +
2014-07-23 20:14:51
neologix
set
files: + socketpair-3.diff
messages: +
title: implement socketpair() on Windows -> add a fallback socketpair() implementation in test.support
2014-06-24 04:16:42
pturing
set
nosy: + pturing
2013-11-30 15:39:23
neologix
set
files: + socketpair-1.diff
messages: +
2013-08-29 18:24:15
neologix
set
messages: +
2013-08-29 17:27:34
python-dev
set
nosy: + python-dev
messages: +
2013-08-28 21:29:35
neologix
set
messages: +
2013-08-28 21:01:41
pitrou
set
messages: +
2013-08-28 20:56:56
vstinner
set
messages: +
2013-08-28 20:54:54
neologix
set
nosy: + pitrou
messages: +
2013-08-28 19:50:44
vstinner
set
messages: +
2013-08-28 18:27:29
neologix
set
messages: +
2013-08-25 10:10:37
neologix
set
files: + sendall_write.diff
2013-08-25 10:10:09
neologix
set
messages: +
2013-08-24 22:42:00
neologix
set
messages: +
2013-08-24 13:52:12
vstinner
set
messages: +
2013-08-24 10:44:34
neologix
set
messages: +
2013-08-24 10:16:57
vstinner
set
messages: +
2013-08-24 08:37:01
neologix
set
files: + socketpair.diff
versions: + Python 3.4
messages: +
keywords: + patch, needs review
stage: needs patch -> patch review
2013-08-16 15:52:06
sbt
set
messages: +
2013-08-16 10:48:20
neologix
set
messages: +
2013-08-09 20:28:47
vstinner
set
nosy: + vstinner
2013-08-03 12:11:35
neologix
create