Use ConnectEx on Windows, to do async connect of sockets.

Use ConnectEx on Windows, to do async connect of sockets.

BUG=https://code.google.com/p/dart/issues/detail?id=18154
R=sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=35622

[Søren Gjesse, 2014-05-01 07:19:25]

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/eventhandler...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/eventhandler...
runtime/bin/eventhandler_win.cc:823: if (ok || WSAGetLastError() !=
WSA_IO_PENDING) {
When is it possible to get a success here? Please add a comment on this.

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/eventhandler...
runtime/bin/eventhandler_win.cc:843: OverlappedBuffer::DisposeBuffer(buffer);
Please add a comment on this call, e.g. " // Update the accepted socket to
support the full range of API calls."

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:131: intptr_t Socket::Create(RawAddr addr) {
Rename this to CreateBind to match the naming.

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:163: }
Can't you use the same code as is used for the other bind calls?

  status = bind(s,
                &addr->addr,
                SocketAddress::GetAddrLength(addr));

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:164: if (status != NO_ERROR) {
This should not be fatal.

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:182: int status = WSAIoctl(s,
Are you sure you cannot just load this once like it is done with AcceptEx?

[Anders Johnsen, 2014-05-01 08:05:11]

PTAL

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/eventhandler...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/eventhandler...
runtime/bin/eventhandler_win.cc:823: if (ok || WSAGetLastError() !=
WSA_IO_PENDING) {
On 2014/05/01 07:19:26, Søren Gjesse wrote:
> When is it possible to get a success here? Please add a comment on this.

"On success, the DisconnectEx function returns TRUE. On failure, the function
returns FALSE. Use the WSAGetLastError function to get extended error
information. If a call to the WSAGetLastError function returns ERROR_IO_PENDING,
the operation initiated successfully and is in progress. Under such
circumstances, the call may still fail when the operation completes."

So basically just like the other Overlapped APIs we use. Adding comment.

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/eventhandler...
runtime/bin/eventhandler_win.cc:843: OverlappedBuffer::DisposeBuffer(buffer);
On 2014/05/01 07:19:26, Søren Gjesse wrote:
> Please add a comment on this call, e.g. " // Update the accepted socket to
> support the full range of API calls."

Done.

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:131: intptr_t Socket::Create(RawAddr addr) {
On 2014/05/01 07:19:26, Søren Gjesse wrote:
> Rename this to CreateBind to match the naming.

That would force a rename on all platforms, which is not true then. It's an
implementation detail on Windows.

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:163: }
On 2014/05/01 07:19:26, Søren Gjesse wrote:
> Can't you use the same code as is used for the other bind calls?
> 
>   status = bind(s,
>                 &addr->addr,
>                 SocketAddress::GetAddrLength(addr));

Done.

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:164: if (status != NO_ERROR) {
On 2014/05/01 07:19:26, Søren Gjesse wrote:
> This should not be fatal.

Done.

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:182: int status = WSAIoctl(s,
On 2014/05/01 07:19:26, Søren Gjesse wrote:
> Are you sure you cannot just load this once like it is done with AcceptEx?

No, it's using the socket as an argument. Also, it's done per-socket for
AcceptEx as well.

[Søren Gjesse, 2014-05-01 09:42:38]

lgtm

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:131: intptr_t Socket::Create(RawAddr addr) {
On 2014/05/01 08:05:11, Anders Johnsen wrote:
> On 2014/05/01 07:19:26, Søren Gjesse wrote:
> > Rename this to CreateBind to match the naming.
> 
> That would force a rename on all platforms, which is not true then. It's an
> implementation detail on Windows.

Sure. We might want to add separate bind on all platforms at some point to have
connect use a user-specified network interface.

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:182: int status = WSAIoctl(s,
On 2014/05/01 08:05:11, Anders Johnsen wrote:
> On 2014/05/01 07:19:26, Søren Gjesse wrote:
> > Are you sure you cannot just load this once like it is done with AcceptEx?
> 
> No, it's using the socket as an argument. Also, it's done per-socket for
> AcceptEx as well.

Of cause you are right.

[Anders Johnsen, 2014-05-01 10:48:31]

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/264613002/diff/40001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:131: intptr_t Socket::Create(RawAddr addr) {
On 2014/05/01 09:42:38, Søren Gjesse wrote:
> On 2014/05/01 08:05:11, Anders Johnsen wrote:
> > On 2014/05/01 07:19:26, Søren Gjesse wrote:
> > > Rename this to CreateBind to match the naming.
> > 
> > That would force a rename on all platforms, which is not true then. It's an
> > implementation detail on Windows.
> 
> Sure. We might want to add separate bind on all platforms at some point to
have
> connect use a user-specified network interface.

Moving to Connect.

[Anders Johnsen, 2014-05-01 10:49:06]

Committed patchset #5 manually as r35622 (presubmit successful).