This adds the first version of SyncSocket to base, along with a trivial unitt...

This adds the first version of SyncSocket to base, along with a trivial unittest.
SyncSocket provides a blocking send/receive that can be used for synchronization.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32927

[sehr (please use chromium), 2009-11-20 02:34:53]

On 2009/11/20 02:29:23, sehr wrote:
> 

And obviously I haven't added the Mac or Linux support yet.

[Paweł Hajdan Jr., 2009-11-20 07:23:26]

NACK based on high probability of flakiness.

http://codereview.chromium.org/418004/diff/1/6
File ipc/sync_socket_unittest.cc (right):

http://codereview.chromium.org/418004/diff/1/6#newcode182
Line 182: PlatformThread::Sleep(1000);
This is going to be flaky. Please wait properly for some start event or so.

[sehr (please use chromium), 2009-11-20 15:51:59]

FWIW, this was cargo-culted from fuzzer tests :-).  Agreed, though.  I'll
find another way.

On Thu, Nov 19, 2009 at 11:23 PM, <phajdan.jr@chromium.org> wrote:

> NACK based on high probability of flakiness.
>
>
> http://codereview.chromium.org/418004/diff/1/6
> File ipc/sync_socket_unittest.cc (right):
>
> http://codereview.chromium.org/418004/diff/1/6#newcode182
> Line 182: PlatformThread::Sleep(1000);
> This is going to be flaky. Please wait properly for some start event or
> so.
>
>
> http://codereview.chromium.org/418004
>

[cpu_(ooo_6.6-7.5), 2009-11-21 19:19:56]

David, can you take a look at the lint errors ? It looks that some of them are
actual style issues.

[cpu_(ooo_6.6-7.5), 2009-11-23 19:55:22]

http://codereview.chromium.org/418004/diff/1/8
File base/sync_socket.h (right):

http://codereview.chromium.org/418004/diff/1/8#newcode21
base/sync_socket.h:21: public:
indent in public:

http://codereview.chromium.org/418004/diff/1/8#newcode38
base/sync_socket.h:38: int Close();
the return is not a boolean given line 37.

http://codereview.chromium.org/418004/diff/1/8#newcode47
base/sync_socket.h:47: 
so 1) can you send 0 bytes? 2) the mismatch between int and size_t on the
sent/send sizes. 

If the return value is size_t then we could forbid sending zero bytes.

http://codereview.chromium.org/418004/diff/1/8#newcode53
base/sync_socket.h:53: 
same deal here

http://codereview.chromium.org/418004/diff/1/7
File base/sync_socket_win.cc (right):

http://codereview.chromium.org/418004/diff/1/7#newcode20
base/sync_socket_win.cc:20: const size_t kPipePrefixSize = sizeof kPipePrefix /
sizeof kPipePrefix[0];
don't we have a macro for this? like _countof? in base/basic_types.h

http://codereview.chromium.org/418004/diff/1/7#newcode29
base/sync_socket_win.cc:29: const int kInBufferSize = 4096;   // TBD
replace TBD with TODO(user) : blah blah. By the way? what is to be defined?

http://codereview.chromium.org/418004/diff/1/7#newcode38
base/sync_socket_win.cc:38: Handle handles[2];
seems that the pipe name is only used for the CreateFileA, so we can avoid
socket_pair_count and just use rand() or rand_s()

http://codereview.chromium.org/418004/diff/1/7#newcode57
base/sync_socket_win.cc:57: handles[0] = CreateNamedPipeA(
btw the A versions are slower than the W versions. Using CreateNamePipeW is
faster. We normally use the wide versions unless there is a compelling reason
not to.

http://codereview.chromium.org/418004/diff/1/7#newcode75
base/sync_socket_win.cc:75: NULL,           // default security attributes
period at the end of comments

http://codereview.chromium.org/418004/diff/1/7#newcode78
base/sync_socket_win.cc:78: NULL);          // no template file
we probably want SECURITY_SQOS_PRESENT with some appropriate level.

Unless you are relying on the pipe server side to do impersonation. Are you?

If we don't do this it can be possible for one end to steal the other end token,
which can be very bad.

[sehr (please use chromium), 2009-11-23 22:18:09]

Carlos, Pawel,

Thanks for the feedback.  One more version ready.

David

http://codereview.chromium.org/418004/diff/1/8
File base/sync_socket.h (right):

http://codereview.chromium.org/418004/diff/1/8#newcode21
base/sync_socket.h:21: public:
On 2009/11/23 19:55:22, cpu wrote:
> indent in public:

Done.

http://codereview.chromium.org/418004/diff/1/8#newcode38
base/sync_socket.h:38: int Close();
On 2009/11/23 19:55:22, cpu wrote:
> the return is not a boolean given line 37.

Fixed.

http://codereview.chromium.org/418004/diff/1/8#newcode47
base/sync_socket.h:47: 
On 2009/11/23 19:55:22, cpu wrote:
> so 1) can you send 0 bytes? 2) the mismatch between int and size_t on the
> sent/send sizes. 
> 
> If the return value is size_t then we could forbid sending zero bytes.
> 

Agreed.  I have changed it.

http://codereview.chromium.org/418004/diff/1/8#newcode53
base/sync_socket.h:53: 
On 2009/11/23 19:55:22, cpu wrote:
> same deal here

Ditto.

http://codereview.chromium.org/418004/diff/1/7
File base/sync_socket_win.cc (right):

http://codereview.chromium.org/418004/diff/1/7#newcode20
base/sync_socket_win.cc:20: const size_t kPipePrefixSize = sizeof kPipePrefix /
sizeof kPipePrefix[0];
On 2009/11/23 19:55:22, cpu wrote:
> don't we have a macro for this? like _countof? in base/basic_types.h

Of course -- changed.

http://codereview.chromium.org/418004/diff/1/7#newcode29
base/sync_socket_win.cc:29: const int kInBufferSize = 4096;   // TBD
On 2009/11/23 19:55:22, cpu wrote:
> replace TBD with TODO(user) : blah blah. By the way? what is to be defined?

Not needed.  Removed.

http://codereview.chromium.org/418004/diff/1/7#newcode38
base/sync_socket_win.cc:38: Handle handles[2];
On 2009/11/23 19:55:22, cpu wrote:
> seems that the pipe name is only used for the CreateFileA, so we can avoid
> socket_pair_count and just use rand() or rand_s()

Used rand_s.

http://codereview.chromium.org/418004/diff/1/7#newcode57
base/sync_socket_win.cc:57: handles[0] = CreateNamedPipeA(
On 2009/11/23 19:55:22, cpu wrote:
> btw the A versions are slower than the W versions. Using CreateNamePipeW is
> faster. We normally use the wide versions unless there is a compelling reason
> not to.

Replaced with W versions.  I have left it currently using swprintf.

http://codereview.chromium.org/418004/diff/1/7#newcode75
base/sync_socket_win.cc:75: NULL,           // default security attributes
On 2009/11/23 19:55:22, cpu wrote:
> period at the end of comments

Done.

http://codereview.chromium.org/418004/diff/1/7#newcode78
base/sync_socket_win.cc:78: NULL);          // no template file
On 2009/11/23 19:55:22, cpu wrote:
> we probably want SECURITY_SQOS_PRESENT with some appropriate level.
> 
> Unless you are relying on the pipe server side to do impersonation. Are you?
> 
> If we don't do this it can be possible for one end to steal the other end
token,
> which can be very bad.
> 

Changed to use SECURITY_ANONYMOUS.

http://codereview.chromium.org/418004/diff/1/6
File ipc/sync_socket_unittest.cc (right):

http://codereview.chromium.org/418004/diff/1/6#newcode182
ipc/sync_socket_unittest.cc:182: PlatformThread::Sleep(1000);
On 2009/11/20 07:23:27, Paweł Hajdan Jr. wrote:
> This is going to be flaky. Please wait properly for some start event or so.

Removed it.  Thanks for pointing this bit of cargo-culting out.

[cpu_(ooo_6.6-7.5), 2009-11-24 01:50:36]

LGTM

[Paweł Hajdan Jr., 2009-11-24 06:44:23]

After fixing the micro-nits, LGTM.

Thanks!

http://codereview.chromium.org/418004/diff/7001/8007
File base/sync_socket.h (right):

http://codereview.chromium.org/418004/diff/7001/8007#newcode18
base/sync_socket.h:18: 
nit: Unnecessary empty line.

http://codereview.chromium.org/418004/diff/7001/8007#newcode19
base/sync_socket.h:19: namespace base {
nit: But there should be one empty line below.

[sehr (please use chromium), 2009-11-24 16:13:18]

Thanks again, folks.

http://codereview.chromium.org/418004/diff/7001/8007
File base/sync_socket.h (right):

http://codereview.chromium.org/418004/diff/7001/8007#newcode18
base/sync_socket.h:18: 
On 2009/11/24 06:44:23, Paweł Hajdan Jr. wrote:
> nit: Unnecessary empty line.

Done.

http://codereview.chromium.org/418004/diff/7001/8007#newcode19
base/sync_socket.h:19: namespace base {
On 2009/11/24 06:44:23, Paweł Hajdan Jr. wrote:
> nit: But there should be one empty line below.

Done.

[darin (slow to review), 2009-12-14 22:58:40]

Sorry for being late to the party, but SyncSocket seems somewhat similar to
IPC::Channel.  They are both wrappers for a pipe but with different features. 
This CL doesn't explain why we are adding another interface for a pipe.

[cpu_(ooo_6.6-7.5), 2010-01-14 01:57:01]

I can answer one way. It was decided in the meeting of 11-25:

You can find the notes in your e-mail if you search for:
Pepper/NaCl/3D Weekly meeting notes 11/25/09


On 2009/12/14 22:58:40, darin wrote:
> Sorry for being late to the party, but SyncSocket seems somewhat similar to
> IPC::Channel.  They are both wrappers for a pipe but with different features. 
> This CL doesn't explain why we are adding another interface for a pipe.

[darin (slow to review), 2010-01-14 07:14:08]

Ah, the meeting before thanksgiving.  I'm pretty sure I missed that :-(  Oh
well.

-Darin



On Wed, Jan 13, 2010 at 5:57 PM, <cpu@chromium.org> wrote:

> I can answer one way. It was decided in the meeting of 11-25:
>
> You can find the notes in your e-mail if you search for:
> Pepper/NaCl/3D Weekly meeting notes 11/25/09
>
>
> On 2009/12/14 22:58:40, darin wrote:
>
>> Sorry for being late to the party, but SyncSocket seems somewhat similar
>> to
>> IPC::Channel.  They are both wrappers for a pipe but with different
>> features.
>> This CL doesn't explain why we are adding another interface for a pipe.
>>
>
>
>
> http://codereview.chromium.org/418004
>