iocpreactor should use GetAcceptExSockaddrs instead of getpeername for incoming connections

[ghazel]

socket.getpeername() (despite SO_UPDATE_ACCEPT_CONTEXT) does not always work on incoming connections. The result in this failure case is:

print acc_sock.getpeername() #=> (0, '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00')

(you'll notice not only is the IP an integer 0, but the port is a string of 0s) The answer to this problem is to use GetAcceptExSockaddrs instead. I double-checked this, and it works:

print acc_sock.getpeername(), self.reactor.GetAcceptExSockaddrs(buffer)[1]
old:(0, '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00') new:('127.0.0.1', 1619)

I propose this: t.i.iocp\ops.py AcceptOpEx.ovDone:

localaddr, remoteaddr = self.reactor.GetAcceptExSockaddrs(buffer)
self.transport.acceptDone(acc_sock, remoteaddr)

And then the ServerSocket should take the client tuple the same way t.i.tcp Server does. You'll also notice that t.i.tcp also uses the return remote addr from accept().

[ghazel]

There is a branch with this change called 'acceptex-1798'

[Glyph]

I don't know how to review this. Tag, Mr. Windows!

[Jean-Paul Calderone]

This really needs to get reviewed. Two weeks is too long for a branch to hang around without even being commented on. Can someone with a Win32 dev environment and some familiarity with this code please take a look?

[Jean-Paul Calderone]

Another week gone by....

[teratorn]

I'm putting this on my TODO shortlist. Not exactly sure when I will get to it.

[TimothyFitz]

Note that this doesn't build on any buildbots, because they can't find addrinfo.h (which exists as python-src\Modules\addrinfo.h). py-src\Modules isn't on the path, should it be?

[teratorn]

ghazel, I've had a look through the win32 Platform SDK regarding these functions, and I'm curious to know under what conditions getpeername() fails.

Do you think you could come up with a failing test case and post it here?

[ghazel]

The failing test case probably goes something like: Install Win2k SP4 Run a test which checks the local endpoint

I've seen it happen in WinXP as well, but it's much more intermittent.

[ghazel]

I meant remote endpoint, of course.

[teratorn]

ghazel, GetAcceptExSockaddrs() doesn't strike me as an *obviously* correct API to use. The MS docs say it's only to be used in conjunction with AcceptEx(). Perhaps it works by accident, or perhaps iocpreactor is actually using AcceptEx() at some lower Windows layer.

GetAcceptExSockaddrs doc: ​http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winsock/winsock/getacceptexsockaddrs_2.asp

I'm not a WinSock programmer, but I have a *little* bit of a hard time believing that an old, mainstream, documented, function like getpeername() is basically broken on various version of Windows up to and including XP. I'm much more inclined to think that we're misusing the API somehow.

PenguinOfDoom, do you have any thoughts on this?

[PenguinOfDoom]

No, that's how it works. Sockets spat out by AcceptEx are not your normal sockets. They are crazy winsock extension sockets that only support certain operations, getpeername not included. See the AcceptEx page for details. iocpreactor2 handles this case.

[radix]

ping!

what's the current status of this ticket? It's got the "review" tag but nobody seems to be reviewing it.

[Stephen Thorne]

+    // shouldn't we be increfing these?

Find out, Make a decision, remove the comment, then merge.

[PenguinOfDoom]

This is fixed in the new iocpreactor. See #1760