Implement support for a cancelable SyncSocket.

Implement support for a cancelable SyncSocket.

Currently, blocking SyncSocket operations can not be unblocked from other threads, but this is now supported by using the CancelableSyncSocket class.

The implementation on Mac and Linux is very simple and basically consists of adding a call to shutdown().

On Windows however things are a tiny bit more complex since we use named pipes with synchronous IO and canceling synchronous IO is simply not possible on XP and arguably tricky on Vista+.  So, what we do instead is to use asynchronous IO in a synchronous fashion to support the SyncSocket semantics and as well as allowing the connection to be correctly shut down from another thread.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119051

[Ami GONE FROM CHROMIUM, 2011-12-21 17:41:30]

drive-by

http://codereview.chromium.org/8965053/diff/9002/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

http://codereview.chromium.org/8965053/diff/9002/base/sync_socket_posix.cc#ne...
base/sync_socket_posix.cc:126: return HANDLE_EINTR(shutdown(handle(),
SHUT_RDWR)) >= 0;
What guarantees that shutdown() is thread-safe?

[tommi (sloooow) - chröme, 2011-12-21 20:42:46]

The bsd socket API is generally thread safe with the exception of a few
functions that return pointers to global data (e.g. gethostbyname).
The man page for shutdown() should have more specific information for this
particular case.
As for the SyncSocket class itself and the new derived class, they are not
thread safe, so locking is done (when needed) outside of the class.

Sent from my phone. Pardon brevity.
On Dec 21, 2011 6:41 PM, <fischman@chromium.org> wrote:

> drive-by
>
>
> http://codereview.chromium.**org/8965053/diff/9002/base/**
>
sync_socket_posix.cc<http://codereview.chromium.org/8965053/diff/9002/base/sync_socket_posix.cc>
> File base/sync_socket_posix.cc (right):
>
> http://codereview.chromium.**org/8965053/diff/9002/base/**
>
sync_socket_posix.cc#**newcode126<http://codereview.chromium.org/8965053/diff/9002/base/sync_socket_posix.cc#newcode126>
> base/sync_socket_posix.cc:126: return HANDLE_EINTR(shutdown(handle()**,
> SHUT_RDWR)) >= 0;
> What guarantees that shutdown() is thread-safe?
>
>
http://codereview.chromium.**org/8965053/<http://codereview.chromium.org/8965...
>

[Ami GONE FROM CHROMIUM, 2011-12-21 21:00:58]

On 2011/12/21 20:42:46, tommi wrote:
> The bsd socket API is generally thread safe 

That's what I thought to and sought confirmation on the web but failed to find
authoritative/explicit information.  The man-pages are strangely silent on the
subject of threads (at least socket(2), socket(7), and shutdown(2) are).

[tommi (sloooow) - chröme, 2012-01-23 15:00:07]

ping

[darin (slow to review), 2012-01-24 18:59:03]

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket.h
File base/sync_socket.h (right):

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket.h#...
base/sync_socket.h:41: // |pair| is an array of two closed SyncSockets (they
must not hold a valid
please update this comment to reflect the fact that "pair" is no longer a
parameter.

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket.h#...
base/sync_socket.h:103: static bool CreatePair(CancelableSyncSocket* socket_a,
Other ports don't need to call CreatePair?  SyncSocket::CreatePair doesn't
return CancelableSyncSocket instances.

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket_wi...
File base/sync_socket_win.cc (right):

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket_wi...
base/sync_socket_win.cc:10: using base::win::ScopedHandle;
nit: you could put this inside the "namespace base {" section as just "using
win::ScopedHandle;"

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket_wi...
base/sync_socket_win.cc:177: DWORD chunk = static_cast<DWORD>(
Perhaps an inline helper function would be useful.  Repeating this code snippet
everywhere seems unfortunate.

[tommi (sloooow) - chröme, 2012-01-24 21:03:25]

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket.h
File base/sync_socket.h (right):

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket.h#...
base/sync_socket.h:41: // |pair| is an array of two closed SyncSockets (they
must not hold a valid
On 2012/01/24 18:59:04, darin wrote:
> please update this comment to reflect the fact that "pair" is no longer a
> parameter.

Done.

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket.h#...
base/sync_socket.h:103: static bool CreatePair(CancelableSyncSocket* socket_a,
On 2012/01/24 18:59:04, darin wrote:
> Other ports don't need to call CreatePair?  SyncSocket::CreatePair doesn't
> return CancelableSyncSocket instances.

Other ports don't need their own implementation since the inherited
SyncSocket::CreatePair will do the right thing.  I.e. this will work on linux:

  base::CancelableSyncSocket a, b;
  base::CancelableSyncSocket::CreatePair(&a, &b);

Is this perhaps too confusing?  I could add an explicit method for all platforms
and just call the parent class on platforms other than win.

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket_wi...
File base/sync_socket_win.cc (right):

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket_wi...
base/sync_socket_win.cc:10: using base::win::ScopedHandle;
On 2012/01/24 18:59:04, darin wrote:
> nit: you could put this inside the "namespace base {" section as just "using
> win::ScopedHandle;"

Done.

https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket_wi...
base/sync_socket_win.cc:177: DWORD chunk = static_cast<DWORD>(
On 2012/01/24 18:59:04, darin wrote:
> Perhaps an inline helper function would be useful.  Repeating this code
snippet
> everywhere seems unfortunate.

Done.

[darin (slow to review), 2012-01-24 21:19:36]

On Tue, Jan 24, 2012 at 1:03 PM, <tommi@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/8965053/diff/**
>
16001/base/sync_socket.h<https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket.h>
> File base/sync_socket.h (right):
>
> https://chromiumcodereview.**appspot.com/8965053/diff/**
>
16001/base/sync_socket.h#**newcode41<https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket.h#newcode41>
> base/sync_socket.h:41: // |pair| is an array of two closed SyncSockets
> (they must not hold a valid
> On 2012/01/24 18:59:04, darin wrote:
>
>> please update this comment to reflect the fact that "pair" is no
>>
> longer a
>
>> parameter.
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/8965053/diff/**
>
16001/base/sync_socket.h#**newcode103<https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket.h#newcode103>
> base/sync_socket.h:103: static bool CreatePair(**CancelableSyncSocket*
> socket_a,
> On 2012/01/24 18:59:04, darin wrote:
>
>> Other ports don't need to call CreatePair?  SyncSocket::CreatePair
>>
> doesn't
>
>> return CancelableSyncSocket instances.
>>
>
> Other ports don't need their own implementation since the inherited
> SyncSocket::CreatePair will do the right thing.  I.e. this will work on
> linux:
>
>  base::CancelableSyncSocket a, b;
>  base::CancelableSyncSocket::**CreatePair(&a, &b);
>
> Is this perhaps too confusing?  I could add an explicit method for all
> platforms and just call the parent class on platforms other than win.
>
>
Makes sense.  It is a bit confusing API-wise since the function signatures
differ.
An explicit CreatePair for all ports seems like it might be better.  It
should help
avoid some mystery.

-Darin


>
> https://chromiumcodereview.**appspot.com/8965053/diff/**
>
16001/base/sync_socket_win.cc<https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket_win.cc>
> File base/sync_socket_win.cc (right):
>
> https://chromiumcodereview.**appspot.com/8965053/diff/**
>
16001/base/sync_socket_win.cc#**newcode10<https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket_win.cc#newcode10>
> base/sync_socket_win.cc:10: using base::win::ScopedHandle;
> On 2012/01/24 18:59:04, darin wrote:
>
>> nit: you could put this inside the "namespace base {" section as just
>>
> "using
>
>> win::ScopedHandle;"
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/8965053/diff/**
>
16001/base/sync_socket_win.cc#**newcode177<https://chromiumcodereview.appspot.com/8965053/diff/16001/base/sync_socket_win.cc#newcode177>
> base/sync_socket_win.cc:177: DWORD chunk = static_cast<DWORD>(
> On 2012/01/24 18:59:04, darin wrote:
>
>> Perhaps an inline helper function would be useful.  Repeating this
>>
> code snippet
>
>> everywhere seems unfortunate.
>>
>
> Done.
>
>
https://chromiumcodereview.**appspot.com/8965053/<https://chromiumcodereview....
>

[tommi (sloooow) - chröme, 2012-01-24 21:56:38]

done.

Btw, in my previous patch set I added one expectation to the unit test that
Receive() would return 0 in case of failure (as per the documentation). However,
the bots just pointed out that the posix implementation was returning -1 in that
case, so I fixed that in the current patch set.

[darin (slow to review), 2012-01-25 00:54:59]

LGTM

https://chromiumcodereview.appspot.com/8965053/diff/23005/base/sync_socket.h
File base/sync_socket.h (right):

https://chromiumcodereview.appspot.com/8965053/diff/23005/base/sync_socket.h#...
base/sync_socket.h:104: static bool CreatePair(CancelableSyncSocket* socket_a,
micro-nit:  perhaps it would be good to list this after the destructor just as
it is done for SyncSocket.

[tommi (sloooow) - chröme, 2012-01-25 10:49:13]

https://chromiumcodereview.appspot.com/8965053/diff/23005/base/sync_socket.h
File base/sync_socket.h (right):

https://chromiumcodereview.appspot.com/8965053/diff/23005/base/sync_socket.h#...
base/sync_socket.h:104: static bool CreatePair(CancelableSyncSocket* socket_a,
On 2012/01/25 00:54:59, darin wrote:
> micro-nit:  perhaps it would be good to list this after the destructor just as
> it is done for SyncSocket.

Done.

[darin (slow to review), 2012-01-25 15:59:31]

LGTM