classification
Title: fromfd() and dup() for _socket on WIndows
Type: behavior Stage:
Components: Versions: Python 2.7
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: Arthur.Kantor, christian.heimes, gvanrossum, nnorwitz, orsenthil, planders, roudkerk, theller
Priority: normal Keywords: patch

Created on 2007-11-03 15:24 by roudkerk, last changed 2012-05-09 08:34 by Arthur.Kantor. This issue is now closed.

Files
File name Uploaded Description Edit
socket_fromfd.patch roudkerk, 2007-11-03 15:24
trunk_socket_fromfd.patch christian.heimes, 2007-11-09 21:40
py3k_socket_fromfd.patch christian.heimes, 2007-11-14 19:19
socket.diff gvanrossum, 2007-11-15 00:46
py3k_another_socket.patch christian.heimes, 2007-11-15 19:33
socket2.diff gvanrossum, 2007-11-15 20:21
socket3.diff gvanrossum, 2007-11-15 20:36
set_error.patch roudkerk, 2007-11-16 00:19
Messages (32)
msg57084 - (view) Author: roudkerk (roudkerk) Date: 2007-11-03 15:24
The patch adds support for _socket.fromfd() and _socket.socket.dup() on
Windows.  It uses the Win32 DuplicateHandle() function.

The patch is to socketmodule.c an test_socket.py.
msg57336 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-11-09 21:40
I've created a clean patch and tested it. It works as promised, great
work! Somebody with more Windows Fu than me should verify it before it's
applied.
msg57492 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-14 18:13
I'd like to see this applied, HOWEVER I'm confused.  I don't see any
mention of fromfd in the patch except in a comment.  Now, in 3.0,
fromfd() is implemented in socket.py using os.dup(); but in 2.6 i don't
see it there either.  What's going in?

I am hoping that this will help issue 1439 along as well.
msg57494 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-14 18:25
Never mind. I must've searched for fromdf.  Crys, I think this is good
to go into 2.6 and be merged into 3.0.  Then in 3.0 we can simplify Bill
Janssen's patch further.
msg57498 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-11-14 18:45
Neal, Thomas, what do you think about the patch? Your knowledge of the
Windows API is greater than mine. Is the duplicate_socket() function ok?
I don't want to apply a patch I don't fully understand.

+#ifdef MS_WINDOWS
+/* On Windows a socket is really a handle not an fd */
+static SOCKET
+duplicate_socket(SOCKET handle)
+{
+       HANDLE newhandle;
+
+       if (!DuplicateHandle(GetCurrentProcess(), (HANDLE)handle,
+                            GetCurrentProcess(), &newhandle,
+                            0, FALSE, DUPLICATE_SAME_ACCESS))
+       {
+               WSASetLastError(WSAEBADF);
+               return INVALID_SOCKET;
+       }
+       return (SOCKET)newhandle;
+}
+#define dup(fd) duplicate_socket(fd)
+#define SOCKETCLOSE closesocket
+#define NO_MAKEFILE /* socket handles can't be treated like file handles */
+#endif
msg57501 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-11-14 19:19
Port of the socket_fromfd.patch to py3k
msg57504 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2007-11-14 19:28
Christian Heimes schrieb:
> Neal, Thomas, what do you think about the patch? Your knowledge of the
> Windows API is greater than mine. Is the duplicate_socket() function ok?
> I don't want to apply a patch I don't fully understand.
> 
> +#ifdef MS_WINDOWS
> +/* On Windows a socket is really a handle not an fd */
> +static SOCKET
> +duplicate_socket(SOCKET handle)
> +{
> +       HANDLE newhandle;
> +
> +       if (!DuplicateHandle(GetCurrentProcess(), (HANDLE)handle,
> +                            GetCurrentProcess(), &newhandle,
> +                            0, FALSE, DUPLICATE_SAME_ACCESS))
> +       {
> +               WSASetLastError(WSAEBADF);
> +               return INVALID_SOCKET;
> +       }
> +       return (SOCKET)newhandle;
> +}
> +#define dup(fd) duplicate_socket(fd)
> +#define SOCKETCLOSE closesocket
> +#define NO_MAKEFILE /* socket handles can't be treated like file handles */
> +#endif

Not much time, so only one comment:
Is it wise to call WSASetLastError(WSAEBADF) instead of calling GetLastError()?

Thomas
msg57505 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-14 19:33
I'm disappointed that this doesn't implement socket.fromfd() properly.
Perhaps fromfd() needs to be implemented in socketmodule.c?

Also, the whole mess with the _base slot can be gotten rid of;
accept() can use the dup() method instead.
msg57506 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-11-14 20:30
> I'm disappointed that this doesn't implement socket.fromfd() properly.
> Perhaps fromfd() needs to be implemented in socketmodule.c?

On Windows socket.fromfd() is not possible. Socket handlers and file
descriptors are different types using different API methods. Other
examples for the discrepancy are select.select() and select.poll(). On
Windows they don't work on file.
msg57508 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-14 21:28
> On Windows socket.fromfd() is not possible. Socket handlers and file
> descriptors are different types using different API methods. Other
> examples for the discrepancy are select.select() and select.poll(). On
> Windows they don't work on file.

I know, but, I was thinking of it taking the fileno() of some other
socket object as argument.

In any case I think the test should not be based on whether os.name ==
'nt' but on the presence of something in the _socket module.
msg57511 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-14 23:13
Crys, did you actually run the tests for the 3.0 port of the patch? I
think accept() is broken, as it checks for _can_dup_socket and will
attempt to call os.dup().

I'm thinking that we should change the C class to not create new objects
on accept() and dup() -- instead, there should be methods _accept() and
_dup() that return new file descriptors.  Then the subclass can
implement accept() and dup() properly without needing to call dup()
again, and the _base slot will no longer be needed.  Tell you what, I'll
try this and submit a patch for testing on Windows later.
msg57513 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 00:00
Here's a new version along the lines I described. Please test on
Windows. It works on Linux and OSX. What this includes:

- the dup-on-windows implementation by R Oudkerk, with the
GetLastError() suggestion from Thomas Heller.

- Replace the C implementations of dup() and accept() with _dup() and
_accept() that return file descriptors instead of socket objects.

- Implement accept() and dup() in Python using _accept() and _dup().

- Get rid of socket.fromfd(). You can use socket.socket(..., fileno=...)
instead (it doesn't dup though).
msg57514 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 00:46
New version of socket.diff.  The only change is better docstrings for
accept() and dup().
msg57515 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 00:47
PS. socket.diff is for 3.0. I'll backport once this is accepted.
msg57516 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-11-15 01:55
I've reviewed your patch and merged it was some pending changes of my
own. The socket tests are passing on Windows. Great work :)

UPDATE:
* Added PyLong_FromSocket_t and PyLong_AsSocket_t to longobject.h to get
rid of some 64bit warnings
* Use name dup_socket() instead of dup(). For Windows developers it's
too confusing to name it dup().
* Readded fromfd, but this time in C again.
msg57541 - (view) Author: roudkerk (roudkerk) Date: 2007-11-15 18:14
From Guido's patch:
>        if (!DuplicateHandle(GetCurrentProcess(), (HANDLE)handle, 
>                             GetCurrentProcess(), &newhandle,
>                             0, FALSE, DUPLICATE_SAME_ACCESS)) 
>        {
>                WSASetLastError(GetLastError());
>                return INVALID_SOCKET;
>        }

If you are going to use GetLastError() like that then you really
should change set_error() so that it recognizes non-WSA errors.

The solution is easy: rip out the 80+ lines of Windows specific code
in set_error() and replace them by the much simpler

#ifdef MS_WINDOWS
        int err_no = WSAGetLastError();
        /* PyErr_SetExcFromWindowsErr() invokes FormatMessage() which
           recognizes the error numbers used by both GetLastError() 
           and WSAGetLastError() */
        if (err_no)
            return PyErr_SetExcFromWindowsErr(socket_error, err_no);
#endif

Note that if you make makefile() use a duplicate socket then you can
also remove all the reference counting stuff from the socket subclass.
msg57548 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 18:44
roudkerk, can you turn that suggestion into a proper patch?
msg57551 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 19:22
> I've reviewed your patch and merged it was some pending changes of my
> own. The socket tests are passing on Windows. Great work :)

You didn't upload this though.
msg57552 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-11-15 19:33
Oh, I forgot
msg57554 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 19:58
- I'm hoping that Bill can submit his SSL changes first.

- If we make _dup() a module-level function, we can implement fromfd()
in Python. I'll do this now.
msg57555 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 20:21
socket2.diff makes dup() a module-level function in _socket (and hence
in socket.py) and uses that to implement fromfd() in socket.py.

No changes otherwise compared to py3k_another_socket.patch.
msg57556 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 20:36
socket3.diff builds on socket2.diff:

- Rename socket class to Socket; add socket() as a factory function.

- Implement roudkerk's suggestion of using a duplicate socket in
makefile() to get rid of the manual reference counting. Yay!

Note: this dinterferes with Bill Janssen's issue 1451.  Whichever is
checked in last must fix issues with the other.
msg57557 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 20:36
Forgot to upload socket3.diff.
msg57566 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2007-11-15 22:19
I've yet another idea for a tiny improvement. Instead of doing

   newfd = _socket.dup(socket_instance.fileno())

I prefer

   newfd = _socket.dup(socket_instance)

It removes some confusing magic and makes error checking on Windows
slightly easier.
msg57567 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-15 22:22
Hold on, socket3.diff breaks four unit tests:

test_ftplib test_poplib test_smtplib test_urllib2net

> newfd = _socket.dup(socket_instance)

But that doesn't allow fromfd to work. I found some real use cases for
fromfd where the fd is passed in through some other means:

http://www.google.com/codesearch?as_q=socket%5C.fromfd&btnG=Search+Code&as_lang=python
msg57576 - (view) Author: roudkerk (roudkerk) Date: 2007-11-16 00:19
Currently on Windows set_error() make use of a large array which maps 
socket error numbers to error messages.

This patch (against socketmodule.c from Python 2.6) removes that 
array and just lets PyErr_SetExcFromWindowsErr() 
generate the message by using the Win32 function FormatMessage().

It is orthogonal from the other patches discussed here.
msg57577 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-16 00:26
Thanks!  roudkerk's patch is submitted as revision 59004.

BTW I need to go back to the drawing board for the rest of the socket
patch here.  Using dup() in makefile() doesn't work for the
ssl.SSLSocket class.  Maybe the explicit reference counting is the best
we can get.
msg57580 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-16 01:25
I've submitted socket2.diff plus small changes to ssl.py.
This seems the best I can do given that I don't think I can make dup()
work on ssl sockets.
msg93551 - (view) Author: Preston Landers (planders) Date: 2009-10-04 17:39
I'm curious what happened with this issue.  It says closed+accepted but
it doesn't appear to be checked in.  Was there a fatal problem
implementing this feature on Windows?   Is it hung up on the inability
to dup SSL sockets?  

I'm highly interested in deploying Python web servers using FastCGI,
SCGI, WSGI, etc on the Windows server platform.  I wrote a simple
Windows fromfd() patch for Python 2.1 which was successfully used by my
organization for many years.  Now we are trying to move to a newer
version of Python and still facing the need to patch this in.  Just
wondering what happened with this feature and perhaps there is something
I can do to help move it along again.  Many Python projects like flup,
python-scgi, etc would be able to support Windows via this feature.

(PS if this question is more appropriately raised on a mailing list
rather than the issue tracker, I apologize in advance.)
msg94160 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2009-10-17 01:18
On Sun, Oct 04, 2009 at 05:39:26PM +0000, Preston Landers wrote:
> I'm curious what happened with this issue.  It says closed+accepted but
> it doesn't appear to be checked in. 

If you see the report, the last message indicates that it is checked in revision 59004.
msg94257 - (view) Author: Preston Landers (planders) Date: 2009-10-19 19:43
Hmm... revision 59004 appears to be unrelated to the main issue at hand.
  As far as I can tell now the status is this: the dup() and fromfd()
support appears to be present in Python 3000 for Windows - at least the
py3k branch. I didn't check the releases.  But it's not present in 2.6
or trunk.  I guess as far as I'm concerned that's fine. I'm going to use
a patch to 2.6.3 to implement it for my purposes anyway.  Thanks.
msg160260 - (view) Author: Arthur Kantor (Arthur.Kantor) Date: 2012-05-09 08:34
Hi guys

It appears, that this patch was meant to go into both the 2.x and 3.x series, but it never made into 2.x  

Are there still plans to apply it to 2.7?

(apologies if I'm asking this in the wrong forum)
History
Date User Action Args
2012-05-09 08:34:16Arthur.Kantorsetnosy: + Arthur.Kantor

messages: + msg160260
versions: + Python 2.7, - Python 2.6
2009-10-19 19:43:02planderssetmessages: + msg94257
2009-10-17 01:18:01orsenthilsetnosy: + orsenthil
messages: + msg94160
2009-10-04 17:39:25planderssetnosy: + planders
messages: + msg93551
2007-11-16 01:25:50gvanrossumsetstatus: open -> closed
resolution: accepted
messages: + msg57580
2007-11-16 00:26:32gvanrossumsetmessages: + msg57577
2007-11-16 00:19:56roudkerksetfiles: + set_error.patch
messages: + msg57576
2007-11-15 22:22:45gvanrossumsetmessages: + msg57567
2007-11-15 22:19:06christian.heimessetmessages: + msg57566
2007-11-15 20:36:57gvanrossumsetfiles: + socket3.diff
messages: + msg57557
2007-11-15 20:36:18gvanrossumsetmessages: + msg57556
2007-11-15 20:21:13gvanrossumsetfiles: + socket2.diff
messages: + msg57555
2007-11-15 19:58:02gvanrossumsetmessages: + msg57554
2007-11-15 19:33:01christian.heimessetfiles: + py3k_another_socket.patch
messages: + msg57552
2007-11-15 19:22:54gvanrossumsetmessages: + msg57551
2007-11-15 18:44:54gvanrossumsetmessages: + msg57548
2007-11-15 18:14:06roudkerksetmessages: + msg57541
2007-11-15 01:55:07christian.heimessetmessages: + msg57516
2007-11-15 00:47:27gvanrossumsetmessages: + msg57515
2007-11-15 00:46:31gvanrossumsetfiles: - socket.diff
2007-11-15 00:46:19gvanrossumsetfiles: + socket.diff
messages: + msg57514
2007-11-15 00:00:46gvanrossumsetfiles: + socket.diff
messages: + msg57513
2007-11-14 23:13:19gvanrossumsetmessages: + msg57511
2007-11-14 21:28:57gvanrossumsetmessages: + msg57508
2007-11-14 20:30:58christian.heimessetmessages: + msg57506
2007-11-14 19:33:17gvanrossumsetmessages: + msg57505
2007-11-14 19:28:59thellersetmessages: + msg57504
2007-11-14 19:19:25christian.heimessetfiles: + py3k_socket_fromfd.patch
messages: + msg57501
2007-11-14 18:45:05christian.heimessetnosy: + nnorwitz, theller
messages: + msg57498
2007-11-14 18:25:29gvanrossumsetassignee: christian.heimes
messages: + msg57494
2007-11-14 18:13:51gvanrossumsetmessages: + msg57492
2007-11-09 21:40:49christian.heimessetfiles: + trunk_socket_fromfd.patch
nosy: + christian.heimes
messages: + msg57336
2007-11-04 18:01:18gvanrossumsetnosy: + gvanrossum
2007-11-04 13:12:11roudkerksetversions: + Python 2.6, - Python 2.5
2007-11-03 15:42:11loewissetkeywords: + patch
2007-11-03 15:24:15roudkerkcreate