msg155842 - (view) |
Author: Kristján Valur Jónsson (kristjan.jonsson) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2012-03-31 11:34 |
Still no responses? I'll submit this then, unless someone objects.
|
msg157177 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2012-04-06 22:51 |
Could you possibly be any more specific? It works for me.
|
msg157709 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
Date: 2012-04-18 20:26 |
Can this issue be reclosed now?
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:28 | admin | set | github: 58518 |
2012-04-18 20:29:37 | pitrou | set | status: open -> closed |
2012-04-18 20:26:20 | sbt | set | messages:
+ msg158657 |
2012-04-07 20:39:53 | python-dev | set | messages:
+ msg157758 |
2012-04-07 20:10:18 | pitrou | set | status: closed -> open assignee: kristjan.jonsson messages:
+ msg157756
stage: resolved |
2012-04-07 11:25:37 | kristjan.jonsson | set | status: open -> closed resolution: fixed |
2012-04-07 11:24:49 | python-dev | set | nosy:
+ python-dev messages:
+ msg157728
|
2012-04-07 10:22:06 | pitrou | set | messages:
+ msg157721 |
2012-04-07 01:24:42 | kristjan.jonsson | set | files:
+ duplicate.patch
messages:
+ msg157713 |
2012-04-07 01:18:15 | kristjan.jonsson | set | files:
- duplicate.patch |
2012-04-06 22:55:12 | pitrou | set | messages:
+ msg157709 |
2012-04-06 22:51:15 | kristjan.jonsson | set | messages:
+ msg157708 |
2012-04-06 22:40:14 | pitrou | set | messages:
+ msg157706 |
2012-04-06 22:32:56 | kristjan.jonsson | set | files:
+ duplicate.patch
messages:
+ msg157705 |
2012-04-06 22:29:10 | kristjan.jonsson | set | files:
- duplicate.patch |
2012-04-06 22:29:04 | kristjan.jonsson | set | files:
- duplicate.patch |
2012-04-06 22:20:49 | pitrou | set | messages:
+ msg157703 |
2012-04-06 22:18:21 | kristjan.jonsson | set | files:
+ duplicate.patch
messages:
+ msg157701 |
2012-04-06 21:48:14 | pitrou | set | messages:
+ msg157697 |
2012-04-06 21:36:25 | kristjan.jonsson | set | files:
+ duplicate.patch
messages:
+ msg157692 |
2012-04-06 14:15:22 | kristjan.jonsson | set | messages:
+ msg157666 |
2012-04-06 12:08:27 | pitrou | set | messages:
+ msg157663 |
2012-04-06 11:59:04 | pitrou | set | messages:
+ msg157662 |
2012-04-06 11:50:35 | kristjan.jonsson | set | messages:
+ msg157661 |
2012-04-06 11:46:40 | kristjan.jonsson | set | messages:
- msg157189 |
2012-04-05 22:22:16 | santoso.wijaya | set | nosy:
+ santoso.wijaya
|
2012-03-31 13:37:43 | kristjan.jonsson | set | messages:
+ msg157189 |
2012-03-31 11:59:01 | pitrou | set | nosy:
+ pitrou messages:
+ msg157177
|
2012-03-31 11:34:37 | kristjan.jonsson | set | messages:
+ msg157171 |
2012-03-24 10:17:30 | kristjan.jonsson | set | messages:
+ msg156687 versions:
+ Python 3.3, - Python 3.4 |
2012-03-20 15:12:24 | kristjan.jonsson | set | messages:
- msg156418 |
2012-03-20 15:12:07 | kristjan.jonsson | set | files:
- duplicate.patch |
2012-03-20 15:11:25 | kristjan.jonsson | set | files:
+ duplicate.patch
messages:
+ msg156418 |
2012-03-20 15:09:16 | kristjan.jonsson | set | files:
+ duplicate.patch
messages:
+ msg156417 |
2012-03-19 21:42:08 | sbt | set | messages:
+ msg156360 |
2012-03-19 19:18:35 | kristjan.jonsson | set | messages:
+ msg156356 |
2012-03-19 19:01:20 | sbt | set | messages:
+ msg156354 |
2012-03-19 17:59:38 | kristjan.jonsson | set | files:
+ duplicate.patch
messages:
+ msg156351 |
2012-03-19 14:38:38 | kristjan.jonsson | set | messages:
+ msg156343 |
2012-03-19 14:19:54 | kristjan.jonsson | set | messages:
+ msg156342 |
2012-03-19 13:48:26 | kristjan.jonsson | set | files:
+ duplicate.patch
messages:
+ msg156339 |
2012-03-15 12:09:44 | sbt | set | nosy:
+ sbt messages:
+ msg155880
|
2012-03-15 09:43:00 | giampaolo.rodola | set | nosy:
+ giampaolo.rodola
|
2012-03-15 01:38:06 | kristjan.jonsson | create | |