classification
Title: Socket duplication for windows
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: kristjan.jonsson Nosy List: giampaolo.rodola, kristjan.jonsson, pitrou, python-dev, santoso.wijaya, sbt
Priority: normal Keywords: patch

Created on 2012-03-15 01:38 by kristjan.jonsson, last changed 2012-04-18 20:29 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
duplicate.patch kristjan.jonsson, 2012-03-15 01:38 review
duplicate.patch kristjan.jonsson, 2012-03-19 13:48 New version of patch review
duplicate.patch kristjan.jonsson, 2012-03-19 17:59 third version of patch review
duplicate.patch kristjan.jonsson, 2012-03-20 15:09 review
duplicate.patch kristjan.jonsson, 2012-04-07 01:24 review
Messages (31)
msg155842 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-03-15 01:38
winsock natively supports the duplciation of sockets for use on different processes.  This patch proposes to add that functionality:
socket.duplicate(target_pid) gets a bytes representation of the duplicate socket, usable for the target process.
socket.socket(x,y,z,data) creates the socket object from the bytes representation.

The patch contains a test using multiprocessing that works.
Note that multiprocessing already contains its own code to achieve this, and that code can possibly be simplified with this patch.

Note also the new function "duplicate."  Perhaps "dup(target_pid) would be better?  But it would have different semantics.  
Also notice how we overload the "fromfd" parameter in socket.socket() to recreate a socket from a bytes representation.  Maybe this is not ideal?
Looking for thoughs here.
msg155880 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-03-15 12:09
It appears that the 4th argument of the socket constructor is undocumented, so presumably one is expected to use fromfd() instead.

Maybe you could have a frominfo(info) function (to match fromfd(fd,...)) and a dupinfo(pid) method.

(It appears that multiprocessing uses DuplicateHandle() instead of WSADuplicateSocket() for duplicating socket handles on Windows.  That should be fixed.)
msg156339 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-03-19 13:48
I'm adding an updated version of the patch, taking into account issues raised from reviewers.  No documentation yet.
msg156342 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-03-19 14:19
I like the idea of having a frominfo() function.  Though "info" is maybe too generic?  Also, dupinfo() is probably better than duplicate, even though there is no dup involved.

But I also wonder if we can make this windows/unix agnostic?
on unix, you would achieve the same thing by:
1) getting fd with fileno()
2) sending it over a unix domain socket using sendmsg() (used for duplicating fds betwerrn processes
3) calling fromfd() on the target host.

We could have a uniform process (more or less) if dupinfo(pid) would return a string containing the fileno on linux, and frominfo() were to accept such a string too.  The differeince between linux/windows would then be that linux needs to transfer the "info" between processes using the "magic" sendmsg() control info ona UNIX domain sock, whereas on Windows one can use whatever RPC one wishes.  To implement this, we could therefore have special "sendinfo" and "recvinfo" functions.

The process would then become, on either platform:
1) info = dupinfo(pid)
2) sendinfo(info)/info=recvinfo()
3) frominfo(info)

But again: "info" is very generic here.  any suggestions for a better name?
msg156343 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-03-19 14:38
On second thought, sendinfo/recvinfo makes no sense.  On unix, one has to create and manage the necessary unix domain sockets, and such code belongs higher up somewhere.  So dupinfo(pid), and frominfo() I'll add.
msg156351 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-03-19 17:59
Here is a new version of thepatch.  We now have socket.socket.share(pid) and socket.fromshare(data) to do these.
I think this captures the functionality better than "duplicate" or duppid() since there is no actual duplication involved until the fromshare() function is called.

Still no documentation.  What do you think?
msg156354 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-03-19 19:01
> I think this captures the functionality better than "duplicate" or 
> duppid() since there is no actual duplication involved until the 
> fromshare() function is called.

Are you saying the WSADuplicateSocket() call in share() doesn't duplicate the handle in to the target process?  I am pretty sure it does.

(Delaying handle duplication till WSASocket() is called in the target process would be rather problematic since then you cannot close the original socket in the source processes until you know the duplication has occurred.)
msg156356 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-03-19 19:18
Good point.  The docs don't actually say, except that the docs show a pretty elaborate process dance, where the source process only closes its side of the socket once the target process has called WSASocket().

If duplication happened early, then there would have to be a way to "unduplicate" it in the source process if, say, IPC somehow failed.  There is currently no api to undo the effects of WSADuplicateSocket().

It's easy enough to test, of course.  I´ll do some test using my unittest.
msg156360 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-03-19 21:42
> If duplication happened early, then there would have to be a way to 
> "unduplicate" it in the source process if, say, IPC somehow failed.  
> There is currently no api to undo the effects of WSADuplicateSocket().

If this were a normal handle then you could use the DUPLICATE_CLOSE_SOURCE flag with DuplicateHandle() to close it.  But using DuplicateHandle() with socket handles is discouraged.

I find the ability to duplicate and close handles in unrelated processes of the same user rather surprising.
msg156417 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-03-20 15:09
A new patch with fixes and documentation.
And yes, socket.share() does in fact perform duplication, because socket.close() can be performed immediately afterwards.
msg156687 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-03-24 10:17
So, what is the consensus, does this go in or out?
What about the "share()" and "fromshare()" functions, are people ok with that?
msg157171 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-03-31 11:34
Still no responses?  I'll submit this then, unless someone objects.
msg157177 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-03-31 11:59
I would like to see this discussed on python-dev.
msg157661 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-06 11:50
Not much happening on python-dev.
Perhaps this is not so controversial.

http://www.mail-archive.com/python-dev@python.org/msg66206.html
msg157662 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-06 11:59
I have one question: you're simply doing a binary copy of the WSAPROTOCOL_INFO structure. Does it mean sharing wouldn't work between e.g. different MSVC builds of Python (if the structure is different), or between a 32-bit and a 64-bit Python?
msg157663 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-06 12:08
Comments about the patch:
- versionadded should be 3.3, not 3.4
- MultiprocessingFunc would deserve a meaningful, PEP8 name; it should also probably be a classmethod on TestSocketSharing
- please use PEP8 for comments (a space after the "#")
- it would be nice to have a test for the ValueError when passing a bytes object of the wrong size
- also, there should be a test that the "family", "type" and "proto" attributes are correctly set on the fromshare() result (perhaps "timeout" too)
msg157666 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-06 14:15
The binary copy:
This is a winsock2 feature.  There is no difference in layout between 32 bits and 64 bits.  I also don't think there is a difference between SDK versions.  However, strangely, there is a difference between unicode and non-unicode builds.  But since we are using this in python3, which always uses the same unicode setting, this shouldn't be an issue.

I'll take your other comments into account and produce a new patch.
Similar to socket.dup(), the socket shared is assumed to be in "blockin" mode (this his how its timeout is initialized).  We can test for that too.
msg157692 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-06 21:36
Here is a new patch, with more tests.
Note that the process worker function can't be a member function because of how multiprocessing works on Windows.
msg157697 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-06 21:48
> Here is a new patch, with more tests.
> Note that the process worker function can't be a member function
> because of how multiprocessing works on Windows.

Some comments:
- I said classmethod, not member function; this is how
test_multiprocessing works, so it should be possible...
- why did you change the gethostbyname() tests?
- before using AF_INET6, you might have to test that IPv6 is available
on the test machine (I think there are variables / functions for that in
test_socket)
- in compareSockets:

+        if org.proto != 0:
+            self.assertEqual(sock.proto, other.proto)

`sock` doesn't seem to exist at all...
msg157701 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-06 22:18
by "memberfunction" I mean a member of the class.  I tried staticmethod and it didn't work:
I tried a staticmethod and it didn't work:
======================================================================
ERROR: testShare (test.test_socket.TestSocketSharing)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\pydev\cpython\lib\test\test_socket.py", line 4723, in testShare
    p.start()
  File "D:\pydev\cpython\lib\multiprocessing\process.py", line 136, in start
    self._popen = Popen(self)
  File "D:\pydev\cpython\lib\multiprocessing\forking.py", line 269, in __init__
    dump(process_obj, to_child, HIGHEST_PROTOCOL)
  File "D:\pydev\cpython\lib\multiprocessing\forking.py", line 190, in dump
    ForkingPickler(file, protocol).dump(obj)
_pickle.PicklingError: Can't pickle <class 'function'>: attribute lookup builtins.function failed

(Picking this kind of stuff should work of course but that's another story)
But changing it to staticmethod strangely fixes it.  Interesting.

the gethostbynametests were an encoding mistake.  Would not have been checked in but it was nice of you to notice.

These are windows only tests for python 3.3.  There is no need to test for IPV6.

Here is yet another patch.
msg157703 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-06 22:20
[...]
> But changing it to staticmethod strangely fixes it.  Interesting.

Sorry?

> Here is yet another patch.

Your patch is broken, it has lots of duplicate stuff...
msg157705 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-06 22:32
>> But changing it to staticmethod strangely fixes it.  Interesting.
>Sorry?
Don't be, probably not your fault.  Today we have learned that we can spawn multipropcessing targets with classmethods, but not staticmethods.

>Your patch is broken, it has lots of duplicate stuff...
Yes, I'm sorry, it was a merging error.  I'm still getting to grips with Hg.  I removed the last two and added a new one.  Hope you like it.
msg157706 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-06 22:40
> Yes, I'm sorry, it was a merging error.  I'm still getting to grips
> with Hg.  I removed the last two and added a new one.  Hope you like
> it.

It's still broken.
msg157708 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-06 22:51
Could you possibly be any more specific?  It works for me.
msg157709 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-06 22:55
> Could you possibly be any more specific?  It works for me.

Just read it by yourself. There are still duplicate portions there:
http://bugs.python.org/file25146/duplicate.patch

In general, it's nicer to others to review your changes before uploading them.
msg157713 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-07 01:24
Well, I think I have all of the merge errors out now.  You´ll forgive me if I didn't notice them all at first, but this is why we review code and you would have helped me by giving me specifics, since after a time, the eyes tend to glaze over.
Any other comments?
msg157721 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-07 10:22
> Any other comments?

No, the patch looks ok now. Please watch the buildbots after you commit.
msg157728 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-07 11:24
New changeset 51b4bddd0e92 by Kristján Valur Jónsson in branch 'default':
Issue #14310: inter-process socket duplication for windows
http://hg.python.org/cpython/rev/51b4bddd0e92
msg157756 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-07 20:10
Re-opening. There is now a buildbot failure:

======================================================================
ERROR: testTypes (test.test_socket.TestSocketSharing)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\test\test_socket.py", line 4738, in testTypes
    source = socket.socket(f, t)
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\socket.py", line 94, in __init__
    _socket.socket.__init__(self, family, type, proto, fileno)
OSError: [Error 10047] An address incompatible with the requested protocol was used

http://www.python.org/dev/buildbot/all/builders/x86%20XP-4%203.x/
msg157758 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-07 20:39
New changeset 9b858096044e by Kristján Valur Jónsson in branch 'default':
Issue #14310: Catch testing errors when trying to create unsupported socket
http://hg.python.org/cpython/rev/9b858096044e
msg158657 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-18 20:26
Can this issue be reclosed now?
History
Date User Action Args
2012-04-18 20:29:37pitrousetstatus: open -> closed
2012-04-18 20:26:20sbtsetmessages: + msg158657
2012-04-07 20:39:53python-devsetmessages: + msg157758
2012-04-07 20:10:18pitrousetstatus: closed -> open
assignee: kristjan.jonsson
messages: + msg157756

stage: resolved
2012-04-07 11:25:37kristjan.jonssonsetstatus: open -> closed
resolution: fixed
2012-04-07 11:24:49python-devsetnosy: + python-dev
messages: + msg157728
2012-04-07 10:22:06pitrousetmessages: + msg157721
2012-04-07 01:24:42kristjan.jonssonsetfiles: + duplicate.patch

messages: + msg157713
2012-04-07 01:18:15kristjan.jonssonsetfiles: - duplicate.patch
2012-04-06 22:55:12pitrousetmessages: + msg157709
2012-04-06 22:51:15kristjan.jonssonsetmessages: + msg157708
2012-04-06 22:40:14pitrousetmessages: + msg157706
2012-04-06 22:32:56kristjan.jonssonsetfiles: + duplicate.patch

messages: + msg157705
2012-04-06 22:29:10kristjan.jonssonsetfiles: - duplicate.patch
2012-04-06 22:29:04kristjan.jonssonsetfiles: - duplicate.patch
2012-04-06 22:20:49pitrousetmessages: + msg157703
2012-04-06 22:18:21kristjan.jonssonsetfiles: + duplicate.patch

messages: + msg157701
2012-04-06 21:48:14pitrousetmessages: + msg157697
2012-04-06 21:36:25kristjan.jonssonsetfiles: + duplicate.patch

messages: + msg157692
2012-04-06 14:15:22kristjan.jonssonsetmessages: + msg157666
2012-04-06 12:08:27pitrousetmessages: + msg157663
2012-04-06 11:59:04pitrousetmessages: + msg157662
2012-04-06 11:50:35kristjan.jonssonsetmessages: + msg157661
2012-04-06 11:46:40kristjan.jonssonsetmessages: - msg157189
2012-04-05 22:22:16santoso.wijayasetnosy: + santoso.wijaya
2012-03-31 13:37:43kristjan.jonssonsetmessages: + msg157189
2012-03-31 11:59:01pitrousetnosy: + pitrou
messages: + msg157177
2012-03-31 11:34:37kristjan.jonssonsetmessages: + msg157171
2012-03-24 10:17:30kristjan.jonssonsetmessages: + msg156687
versions: + Python 3.3, - Python 3.4
2012-03-20 15:12:24kristjan.jonssonsetmessages: - msg156418
2012-03-20 15:12:07kristjan.jonssonsetfiles: - duplicate.patch
2012-03-20 15:11:25kristjan.jonssonsetfiles: + duplicate.patch

messages: + msg156418
2012-03-20 15:09:16kristjan.jonssonsetfiles: + duplicate.patch

messages: + msg156417
2012-03-19 21:42:08sbtsetmessages: + msg156360
2012-03-19 19:18:35kristjan.jonssonsetmessages: + msg156356
2012-03-19 19:01:20sbtsetmessages: + msg156354
2012-03-19 17:59:38kristjan.jonssonsetfiles: + duplicate.patch

messages: + msg156351
2012-03-19 14:38:38kristjan.jonssonsetmessages: + msg156343
2012-03-19 14:19:54kristjan.jonssonsetmessages: + msg156342
2012-03-19 13:48:26kristjan.jonssonsetfiles: + duplicate.patch

messages: + msg156339
2012-03-15 12:09:44sbtsetnosy: + sbt
messages: + msg155880
2012-03-15 09:43:00giampaolo.rodolasetnosy: + giampaolo.rodola
2012-03-15 01:38:06kristjan.jonssoncreate