Introduces ability for session to wait on a migration trigger for a new

Introduces ability for session to wait on a migration trigger for a new
network to become connected.

BUG=

Committed: https://crrev.com/5b7855189df53a2859069d2cbb8f6ee90109960c
Cr-Commit-Position: refs/heads/master@{#418171}

[Jana, 2016-09-11 08:31:27]

Description was changed from

==========
Introduces ability for session to wait on a migration trigger for a new
network to become connected.

sync

BUG=
==========

to

==========
Introduces ability for session to wait on a migration trigger for a new
network to become connected.

BUG=
==========

[Ryan Hamilton, 2016-09-12 03:42:00]

rch@chromium.org changed reviewers:
+ rch@chromium.org

[Ryan Hamilton, 2016-09-12 03:42:01]

Just a couple of questions.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1010: void
QuicChromiumClientSession::OnNoNewNetwork() {
So this is called, obviously, from the write error code path. But it's also
called from any of the various NCN events, right?

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1017:
->set_write_blocked(true);
I don't understand the motivation for this. What happens if we don't do this? it
seems like this will be handled via the HandleWriteError code path.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1072: return;
So does this mean that if multiple timeouts are queued, only the oldest one
fires? I'd think we actually want the most recent to fire.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1092: if (!migration_pending_)
With this CL, migration_pending_ is now true both for the write_error_ case, or
this new delayed case, right? And in both cases, this is guarding the call to
MigrateToNewNetwork, right? (Just confirming).

(I think the sockets_.size() trick would also work here, but I just wanted to
make sure I understood what's going on)

Hm, that's not quite right. The write error case calls
MaybeMigrateSingleSession. But that ends up calling MigrateToNewNetwork?

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1439: }
I expected this to call MigrateOrCloseSessions(), but I'm guessing that's
problematic?

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1475: // @@@ look at other cl for
changes here.
?

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory_test.cc:511: while
(runner_->GetPostedTasks().size() > 0)
nit: instead of size() > 0, !empty()

[Jana, 2016-09-12 23:11:01]

Responding to a few comments

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1010: void
QuicChromiumClientSession::OnNoNewNetwork() {
On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> So this is called, obviously, from the write error code path. But it's also
> called from any of the various NCN events, right?

Yes. It's called only for the DISCONNECTED event, since the other events are
CONNECTED and MADE_DEFAULT, which come with a valid network specified.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1017:
->set_write_blocked(true);
On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> I don't understand the motivation for this. What happens if we don't do this?
it
> seems like this will be handled via the HandleWriteError code path.

This causes an NCN notification to block the socket to avoid any subsequent
writes to this socket. If we don't do this, there may be later write events to a
disconnected socket causing write error to happen.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1072: return;
On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> So does this mean that if multiple timeouts are queued, only the oldest one
> fires? I'd think we actually want the most recent to fire.

I don't think it matters much, since it's either DISCONNECTED or write error
that sets this alarm, and they should be in close proximity (basically a race in
Android between the events.) The way the code is now, it's also easier to
arrange for the first one to continue.

[Ryan Hamilton, 2016-09-13 00:33:19]

Almost there!

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1017:
->set_write_blocked(true);
On 2016/09/12 23:11:01, Jana wrote:
> On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> > I don't understand the motivation for this. What happens if we don't do
this?
> it
> > seems like this will be handled via the HandleWriteError code path.
> 
> This causes an NCN notification to block the socket to avoid any subsequent
> writes to this socket. If we don't do this, there may be later write events to
a
> disconnected socket causing write error to happen.

What makes the socket "disconnected" in this case?

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1439: }
On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> I expected this to call MigrateOrCloseSessions(), but I'm guessing that's
> problematic?

ping

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory_test.cc:511: while
(runner_->GetPostedTasks().size() > 0)
On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> nit: instead of size() > 0, !empty()

ping

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1033: // be set to true here.
I don't understand this comment. Perhaps you can explain why this needs to be
set?

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1085: DLOG(INFO) << "In
OnMigrationTimeout: migration_pending_: " << migration_pending_;
nit: remove this?

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1086: if (!migration_pending_)
As discussed, if two migrations start before the first timeout, then the
connection will close prematurely. the sockets_.size() trick will fix this. Just
recording this for posterity since you're going to do it in a followup.

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1089: migration_pending_ =
false;
Since the connection is about to be deleted, I suspect you don't need these two
lines, right?

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1095: */
What's up with this?

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.h (right):

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:278: // new network to connect.
nit: This comment talks about "any sessions" but presumably it only applies to
this session?

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_stream_factory_test.cc:2733:
DISABLED_MigrateSessionOnWriteErrorNoNewNetworkAsync) {
Let's get this test working.

[Jana, 2016-09-13 01:12:19]

Thanks, Ryan. I've removed the use of migration_alarm_pending_ and replaced it
with sockets_.size(), as well as a few other fixes. PTAL.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1017:
->set_write_blocked(true);
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> On 2016/09/12 23:11:01, Jana wrote:
> > On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> > > I don't understand the motivation for this. What happens if we don't do
> this?
> > it
> > > seems like this will be handled via the HandleWriteError code path.
> > 
> > This causes an NCN notification to block the socket to avoid any subsequent
> > writes to this socket. If we don't do this, there may be later write events
to
> a
> > disconnected socket causing write error to happen.
> 
> What makes the socket "disconnected" in this case?

Something in Android? So this is the case where we aren't trying to write but
receive a notification from NCN that network is disconnected. Presumably, the
socket is borked (or in some indeterminate state), but we may try to write to it
later and go down the HandleWriteError path. It seems to make sense to disallow
any writes while we expect the socket to not be working.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1092: if (!migration_pending_)
On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> With this CL, migration_pending_ is now true both for the write_error_ case,
or
> this new delayed case, right? And in both cases, this is guarding the call to
> MigrateToNewNetwork, right? (Just confirming).

That's correct.

> (I think the sockets_.size() trick would also work here, but I just wanted to
> make sure I understood what's going on)
> 
> Hm, that's not quite right. The write error case calls
> MaybeMigrateSingleSession. But that ends up calling MigrateToNewNetwork?

That's right.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1439: }
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> > I expected this to call MigrateOrCloseSessions(), but I'm guessing that's
> > problematic?
> 
> ping

OnNetworkConnected can be called from the NCN when the current network is the
default and is working just fine. We do not want to migrate every time this
notification is received since OnNetworkCOnnected does not mean that a migration
must happen. It just means that sessions that are pending migration now have a
network to use, which is what the ChromiumClientSession's OnNetworkConnected
ensures.

Note that we do call MaybeMigrateOrCloseSessions in OnNetworkMadeDefault.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1475: // @@@ look at other cl for
changes here.
On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> ?

Leftover comment, removed.

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/2329853002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory_test.cc:511: while
(runner_->GetPostedTasks().size() > 0)
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> On 2016/09/12 03:42:01, Ryan Hamilton wrote:
> > nit: instead of size() > 0, !empty()
> 
> ping

Done.

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1033: // be set to true here.
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> I don't understand this comment. Perhaps you can explain why this needs to be
> set?

Comment changed PTAL.

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1085: DLOG(INFO) << "In
OnMigrationTimeout: migration_pending_: " << migration_pending_;
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> nit: remove this?

Done.

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1086: if (!migration_pending_)
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> As discussed, if two migrations start before the first timeout, then the
> connection will close prematurely. the sockets_.size() trick will fix this.
Just
> recording this for posterity since you're going to do it in a followup.

Changed to using sockets_.size().

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1089: migration_pending_ =
false;
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> Since the connection is about to be deleted, I suspect you don't need these
two
> lines, right?

Yup, removed.

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1095: */
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> What's up with this?

Fixed.

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.h (right):

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:278: // new network to connect.
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> nit: This comment talks about "any sessions" but presumably it only applies to
> this session?

Fixed comment.

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_stream_factory_test.cc:2733:
DISABLED_MigrateSessionOnWriteErrorNoNewNetworkAsync) {
On 2016/09/13 00:33:18, Ryan Hamilton wrote:
> Let's get this test working.

Done.

[Jana, 2016-09-13 02:19:28]

Added 4 more tests.

[Ryan Hamilton, 2016-09-13 02:57:31]

lgtm modulo a number of cosmetic issues.

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1007: 
What happened to all the vertical white space? I found it easier to read in the
old version.

https://codereview.chromium.org/2329853002/diff/140001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1072: // If number of sockets
has increased, this migration task is stale.
s/increased/changed/ I'd say. (And "<" => "!=" below)

https://codereview.chromium.org/2329853002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1089: 
Yey, vertical white space!

https://codereview.chromium.org/2329853002/diff/160001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/160001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1029: // ensure that migration
is pending.
Hm. This is still confusing. I think you're explaining why migration_pending_
might not be set to true already?  But that doesn't seem terribly helpful. I
think the point is that migration_pending_ needs to be true while we're waiting
(either for a PostTask via OnWriteError or an NCN notification) for a migration
to happen. Since this method starts waiting we need to mark that we're waiting.
At that point, I'd probably just drop the comment.

[Jana, 2016-09-13 03:12:14]

Thanks Ryan, comments addressed.

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1007: 
On 2016/09/13 02:57:31, Ryan Hamilton wrote:
> What happened to all the vertical white space? I found it easier to read in
the
> old version.

I wasn't sure if it was too much white space, so I had removed it. Added space.

https://codereview.chromium.org/2329853002/diff/140001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1072: // If number of sockets
has increased, this migration task is stale.
On 2016/09/13 02:57:31, Ryan Hamilton wrote:
> s/increased/changed/ I'd say. (And "<" => "!=" below)

Done. Just curious: why? We aren't expecting this to go in any other direction,
are we?

https://codereview.chromium.org/2329853002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1089: 
On 2016/09/13 02:57:31, Ryan Hamilton wrote:
> Yey, vertical white space!

:-)

https://codereview.chromium.org/2329853002/diff/160001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/160001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1029: // ensure that migration
is pending.
On 2016/09/13 02:57:31, Ryan Hamilton wrote:
> Hm. This is still confusing. I think you're explaining why migration_pending_
> might not be set to true already?  But that doesn't seem terribly helpful. I
> think the point is that migration_pending_ needs to be true while we're
waiting
> (either for a PostTask via OnWriteError or an NCN notification) for a
migration
> to happen. Since this method starts waiting we need to mark that we're
waiting.
> At that point, I'd probably just drop the comment.

Yeah, re-reading it, the comment seems to be going all over the place. Dropping
it. Feel free to propose a better comment if you can think of one.

[Jana, 2016-09-13 03:12:32]

The CQ bit was checked by jri@chromium.org

[Jana, 2016-09-13 03:12:32]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2329853002/#ps180001
(title: "comments addressed")

[commit-bot: I haz the power, 2016-09-13 03:12:53]

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

[Ryan Hamilton, 2016-09-13 03:15:32]

https://codereview.chromium.org/2329853002/diff/140001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2329853002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1072: // If number of sockets
has increased, this migration task is stale.
On 2016/09/13 03:12:14, Jana wrote:
> On 2016/09/13 02:57:31, Ryan Hamilton wrote:
> > s/increased/changed/ I'd say. (And "<" => "!=" below)
> 
> Done. Just curious: why? We aren't expecting this to go in any other
direction,
> are we?

Agreed that "we" are not expecting it to go backwards. 

But as far as this method is concerned, if it *did* go backwards we probably
wouldn't want to proceed, would we? Alternatively, DCHECK_GE(num_sockets,
socket_Size()) or some such?

It really doesn't matter... I'm just neurotic!

[commit-bot: I haz the power, 2016-09-13 04:29:30]

Description was changed from

==========
Introduces ability for session to wait on a migration trigger for a new
network to become connected.

BUG=
==========

to

==========
Introduces ability for session to wait on a migration trigger for a new
network to become connected.

BUG=
==========

[commit-bot: I haz the power, 2016-09-13 04:29:31]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-13 04:31:50]

Description was changed from

==========
Introduces ability for session to wait on a migration trigger for a new
network to become connected.

BUG=
==========

to

==========
Introduces ability for session to wait on a migration trigger for a new
network to become connected.

BUG=

Committed: https://crrev.com/5b7855189df53a2859069d2cbb8f6ee90109960c
Cr-Commit-Position: refs/heads/master@{#418171}
==========

[commit-bot: I haz the power, 2016-09-13 04:31:51]

Patchset 10 (id:??) landed as
https://crrev.com/5b7855189df53a2859069d2cbb8f6ee90109960c
Cr-Commit-Position: refs/heads/master@{#418171}