classification
Title: add a fallback socketpair() implementation to the socket module
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, neologix, pitrou, pturing, python-dev, sbt
Priority: normal Keywords: needs review, patch

Created on 2013-08-03 12:11 by neologix, last changed 2014-10-14 21:24 by neologix. This issue is now closed.

Files
File name Uploaded Description Edit
socketpair.diff neologix, 2013-08-24 08:37 review
sendall_write.diff neologix, 2013-08-25 10:10 review
socketpair-1.diff neologix, 2013-11-30 15:39 review
socketpair-3.diff neologix, 2014-07-23 20:14 review
socketpair-4.diff neologix, 2014-07-23 22:21 review
Messages (28)
msg194252 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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.
msg195317 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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?
msg195343 - (view) Author: Richard Oudkerk (sbt) * (Python committer) 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.
msg196068 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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.
msg196070 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-08-24 10:16
On Linux, many tests of test_socket are failing with the pure Python implementation.
msg196071 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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?
msg196078 - (view) Author: STINNER Victor (haypo) * (Python committer) 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
msg196103 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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
msg196117 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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).
msg196401 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-08-28 18:27
Victor, did you have a chance to test the patch?
msg196409 - (view) Author: STINNER Victor (haypo) * (Python committer) 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.
msg196418 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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?
msg196419 - (view) Author: STINNER Victor (haypo) * (Python committer) 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?
msg196421 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-28 21:01
By alternating between PIPE and SOCK, you get POPK, which is as descriptive as it gets.
msg196424 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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
msg196467 - (view) 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
msg196475 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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).
msg204822 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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.
msg223775 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2014-07-23 20:14
Barring any objection, I'll commit the patch attached within a couple days.
msg223776 - (view) Author: STINNER Victor (haypo) * (Python committer) 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!
msg223788 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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.
msg223797 - (view) Author: STINNER Victor (haypo) * (Python committer) 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.
msg223815 - (view) Author: STINNER Victor (haypo) * (Python committer) 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().
msg223817 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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
msg223820 - (view) Author: STINNER Victor (haypo) * (Python committer) 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.
msg229090 - (view) Author: STINNER Victor (haypo) * (Python committer) 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.
msg229347 - (view) 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
msg229353 - (view) 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
2014-10-14 21:24:28neologixsetstatus: 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:07python-devsetmessages: + msg229353
2014-10-14 20:24:01python-devsetmessages: + msg229347
2014-10-11 14:22:36hayposetmessages: + msg229090
2014-07-24 08:49:10hayposetmessages: + msg223820
2014-07-24 08:11:52neologixsetmessages: + msg223817
2014-07-24 08:01:44hayposetmessages: + msg223815
2014-07-23 23:34:34hayposetmessages: + msg223797
2014-07-23 22:21:34neologixsetfiles: + socketpair-4.diff

messages: + msg223788
2014-07-23 21:34:53hayposetversions: + Python 3.5, - Python 3.4
2014-07-23 20:41:50hayposetmessages: + msg223776
2014-07-23 20:14:51neologixsetfiles: + socketpair-3.diff

messages: + msg223775
title: implement socketpair() on Windows -> add a fallback socketpair() implementation in test.support
2014-06-24 04:16:42pturingsetnosy: + pturing
2013-11-30 15:39:23neologixsetfiles: + socketpair-1.diff

messages: + msg204822
2013-08-29 18:24:15neologixsetmessages: + msg196475
2013-08-29 17:27:34python-devsetnosy: + python-dev
messages: + msg196467
2013-08-28 21:29:35neologixsetmessages: + msg196424
2013-08-28 21:01:41pitrousetmessages: + msg196421
2013-08-28 20:56:56hayposetmessages: + msg196419
2013-08-28 20:54:54neologixsetnosy: + pitrou
messages: + msg196418
2013-08-28 19:50:44hayposetmessages: + msg196409
2013-08-28 18:27:29neologixsetmessages: + msg196401
2013-08-25 10:10:37neologixsetfiles: + sendall_write.diff
2013-08-25 10:10:09neologixsetmessages: + msg196117
2013-08-24 22:42:00neologixsetmessages: + msg196103
2013-08-24 13:52:12hayposetmessages: + msg196078
2013-08-24 10:44:34neologixsetmessages: + msg196071
2013-08-24 10:16:57hayposetmessages: + msg196070
2013-08-24 08:37:01neologixsetfiles: + socketpair.diff
versions: + Python 3.4
messages: + msg196068

keywords: + patch, needs review
stage: needs patch -> patch review
2013-08-16 15:52:06sbtsetmessages: + msg195343
2013-08-16 10:48:20neologixsetmessages: + msg195317
2013-08-09 20:28:47hayposetnosy: + haypo
2013-08-03 12:11:35neologixcreate