Remove deprecated NonThreadSafe usage in net\socket.

Remove deprecated NonThreadSafe usage in net\socket.

Used ThreadChecker over the usually prefered SequenceChecker because
some implementations are thread-affine..

This was scripted (check also added to destructors because NonThreadSafe
implicitly checked on destruction but ThreadChecker doesn't).

BUG=676387
Review-Url: https://codereview.chromium.org/2905183002
Cr-Commit-Position: refs/heads/master@{#475604}
Committed: https://chromium.googlesource.com/chromium/src/+/d43afa1fa4853b90c1437b4ea57bf904106258b2

[gab, 2017-05-25 23:30:44]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-05-25 23:30:59]

gab@chromium.org changed reviewers:
+ rsleevi@chromium.org

[gab, 2017-05-25 23:31:00]

Ryan PTaL, thanks

[commit-bot: I haz the power, 2017-05-25 23:32:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 04:18:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 04:18:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ryan Sleevi, 2017-05-26 11:32:41]

rsleevi@chromium.org changed reviewers:
+ eroman@chromium.org, mmenke@chromium.org

[Ryan Sleevi, 2017-05-26 11:32:42]

I'm going to defer to mmenke or eroman as I'm going to be OOO.

TCPSocketWin was thread-affine, by virtue of the OVERLAPPED usage, but AIUI,
we've switched from using overlapped IO to non-blocking IO (that is, the only
usage I can see is with the overlapped.hEvent), in which case, things should be
more sequence-affine in theory (this is because overlapped IO has a thread
affinity for dispatch)

I don't think the POSIX bits are thread-affine - I think they're fully
sequence-affineable - although it may make sense to have them use the more
restrictive affinity if it matches Windows, to avoid multi-platform errors.

Matt and Eric are good because I believe Matt reviewed the original
overlapped->nonblocking changes (from pmeenan?), and Eric would have a gut
instinct as to whether Windows LSPs-et-al (third-party modules) may be relying
on the thread-affinity, such that even though the API doesn't strictly require
thread-affinity, for stability reasons, we keep it thread-affine.

[mmenke, 2017-05-26 20:12:28]

We still use overlapped writes, which has always struck me as weird. 
Regardless, that does mean we need to be torn down on the same thread any
pending overlapped write was initiated on.

On 2017/05/26 11:32:42, Ryan Sleevi wrote:
> I'm going to defer to mmenke or eroman as I'm going to be OOO.
> 
> TCPSocketWin was thread-affine, by virtue of the OVERLAPPED usage, but AIUI,
> we've switched from using overlapped IO to non-blocking IO (that is, the only
> usage I can see is with the overlapped.hEvent), in which case, things should
be
> more sequence-affine in theory (this is because overlapped IO has a thread
> affinity for dispatch)
> 
> I don't think the POSIX bits are thread-affine - I think they're fully
> sequence-affineable - although it may make sense to have them use the more
> restrictive affinity if it matches Windows, to avoid multi-platform errors.
> 
> Matt and Eric are good because I believe Matt reviewed the original
> overlapped->nonblocking changes (from pmeenan?), and Eric would have a gut
> instinct as to whether Windows LSPs-et-al (third-party modules) may be relying
> on the thread-affinity, such that even though the API doesn't strictly require
> thread-affinity, for stability reasons, we keep it thread-affine.

[gab, 2017-05-29 16:26:53]

On 2017/05/26 20:12:28, mmenke wrote:
> We still use overlapped writes, which has always struck me as weird. 
> Regardless, that does mean we need to be torn down on the same thread any
> pending overlapped write was initiated on.

Is that an l.g.t.m. for this CL which uses ThreadChecker and thus enforces
thread-affinity?

Thanks!

> 
> On 2017/05/26 11:32:42, Ryan Sleevi wrote:
> > I'm going to defer to mmenke or eroman as I'm going to be OOO.
> > 
> > TCPSocketWin was thread-affine, by virtue of the OVERLAPPED usage, but AIUI,
> > we've switched from using overlapped IO to non-blocking IO (that is, the
only
> > usage I can see is with the overlapped.hEvent), in which case, things should
> be
> > more sequence-affine in theory (this is because overlapped IO has a thread
> > affinity for dispatch)
> > 
> > I don't think the POSIX bits are thread-affine - I think they're fully
> > sequence-affineable - although it may make sense to have them use the more
> > restrictive affinity if it matches Windows, to avoid multi-platform errors.
> > 
> > Matt and Eric are good because I believe Matt reviewed the original
> > overlapped->nonblocking changes (from pmeenan?), and Eric would have a gut
> > instinct as to whether Windows LSPs-et-al (third-party modules) may be
relying
> > on the thread-affinity, such that even though the API doesn't strictly
require
> > thread-affinity, for stability reasons, we keep it thread-affine.

[mmenke, 2017-05-30 16:26:02]

On 2017/05/29 16:26:53, gab wrote:
> On 2017/05/26 20:12:28, mmenke wrote:
> > We still use overlapped writes, which has always struck me as weird. 
> > Regardless, that does mean we need to be torn down on the same thread any
> > pending overlapped write was initiated on.
> 
> Is that an l.g.t.m. for this CL which uses ThreadChecker and thus enforces
> thread-affinity?
> 
> Thanks!

That SGTM

[Ryan Sleevi, 2017-05-30 16:31:15]

LGTM % nit:

Updated CL description post-discussion :) (It sounds speculative, but it looks
like the speculation has resolved)

(I was slightly concerned by TCPSocketWin being thread-affine but TCPSocketPosix
not being so, but TCPSocketPosix inherits from SocketPosix which is
thread-affine, thus maintaining their interchangability with the same
assumptions)

[gab, 2017-05-30 16:33:13]

Description was changed from

==========
Remove deprecated NonThreadSafe usage in net\socket.

Used ThreadChecker over the usually prefered SequenceChecker because
sockets + platform specific implementations do sound likely to be
thread-affine, let me know if this isn't the case and merely requires
thread-safety (sequence-affinity).

This was scripted (check also added to destructors because NonThreadSafe
implicitly checked on destruction but ThreadChecker doesn't).

BUG=676387
==========

to

==========
Remove deprecated NonThreadSafe usage in net\socket.

Used ThreadChecker over the usually prefered SequenceChecker because
some implementations are thread-affine..

This was scripted (check also added to destructors because NonThreadSafe
implicitly checked on destruction but ThreadChecker doesn't).

BUG=676387
==========

[gab, 2017-05-30 16:33:24]

On 2017/05/30 16:31:15, Ryan Sleevi wrote:
> LGTM % nit:
> 
> Updated CL description post-discussion :) (It sounds speculative, but it looks
> like the speculation has resolved)

Done, thanks

> 
> (I was slightly concerned by TCPSocketWin being thread-affine but
TCPSocketPosix
> not being so, but TCPSocketPosix inherits from SocketPosix which is
> thread-affine, thus maintaining their interchangability with the same
> assumptions)

[gab, 2017-05-30 16:33:29]

The CQ bit was checked by gab@chromium.org

[mmenke, 2017-05-30 16:33:36]

On 2017/05/30 16:26:02, mmenke wrote:
> On 2017/05/29 16:26:53, gab wrote:
> > On 2017/05/26 20:12:28, mmenke wrote:
> > > We still use overlapped writes, which has always struck me as weird. 
> > > Regardless, that does mean we need to be torn down on the same thread any
> > > pending overlapped write was initiated on.
> > 
> > Is that an l.g.t.m. for this CL which uses ThreadChecker and thus enforces
> > thread-affinity?
> > 
> > Thanks!
> 
> That SGTM

Oops, misunderstood - hadn't realized this already used ThreadChecker.  LGTM!

[commit-bot: I haz the power, 2017-05-30 16:34:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 18:26:27]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1496162009447440, "parent_rev":
"b33cc76060c60ba155fb8698cbc31fe3ba759354", "commit_rev":
"d43afa1fa4853b90c1437b4ea57bf904106258b2"}

[commit-bot: I haz the power, 2017-05-30 18:26:35]

Description was changed from

==========
Remove deprecated NonThreadSafe usage in net\socket.

Used ThreadChecker over the usually prefered SequenceChecker because
some implementations are thread-affine..

This was scripted (check also added to destructors because NonThreadSafe
implicitly checked on destruction but ThreadChecker doesn't).

BUG=676387
==========

to

==========
Remove deprecated NonThreadSafe usage in net\socket.

Used ThreadChecker over the usually prefered SequenceChecker because
some implementations are thread-affine..

This was scripted (check also added to destructors because NonThreadSafe
implicitly checked on destruction but ThreadChecker doesn't).

BUG=676387

Review-Url: https://codereview.chromium.org/2905183002
Cr-Commit-Position: refs/heads/master@{#475604}
Committed:
https://chromium.googlesource.com/chromium/src/+/d43afa1fa4853b90c1437b4ea57b...
==========

[commit-bot: I haz the power, 2017-05-30 18:26:36]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/d43afa1fa4853b90c1437b4ea57b...