Move the "wait for QUIC handshake confirmation" logic to QuicChromiumClientSession::StreamRequest

Move the "wait for QUIC handshake confirmation" logic to QuicChromiumClientSession::StreamRequest and out of the individual stream subclasses.

This will allow a subsequent CL to remove the OnHandshakeConfirmed upcall.

BUG=716563
Review-Url: https://codereview.chromium.org/2848923004
Cr-Commit-Position: refs/heads/master@{#468699}
Committed: https://chromium.googlesource.com/chromium/src/+/d606b6c39dbce82e048ed0a737d56dc7d8b0ea32

[Ryan Hamilton, 2017-05-01 02:17:07]

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

[Ryan Hamilton, 2017-05-01 02:17:08]

xunjieli: PTAL - This is part of a series of CLs I'm working on to remove the
reentrancy in the QUIC code. This makes the "wait for confirmation" logic the
same between HTTP and BiDi, as well as making it possible to remove the
OnHandshakeConfirmed() upcall in a followup.

[xunjieli, 2017-05-01 20:15:06]

Looks great. I really like the new change. It makes the handshake confirmation
logic easier to understand from a consumer point of view.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc:1198:
&spdy_request_headers_frame_length, &header_stream_offset));
Settings header before request headers is much more reasonable. I wonder why
these tests had the order flipped before..

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

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:190: const
base::WeakPtr<QuicChromiumClientSession>& session,
Does |session| always outlive the StreamRequest? Will we have a case where
|session| weak ptr is invalidated? 
If not, we could also use a raw QuicChromiumClientSession* ptr here to simplify
the lifetime relationship between StreamRequest and the Session. Not suggesting
to do it in this CL, but I would like to check to see if this is a viable
option.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:231: if (callback_)
I think callback_ is always valid, right? It's never null-ed once set in the
constructor. Same for line 239 and line 246.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:290: if
(requires_confirmation_)
nit: add curly braces or use early return.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:299: int rv) {
nit: maybe add a DCHECK_NE(ERR_IO_PENDING, rv) here.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:391:
QuicChromiumClientSession::~QuicChromiumClientSession() {
Maybe we can add a DCHECK(waiting_for_confirmation_callbacks_.empty()) here?

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:965:
NotifyAllWaitingForConfirmation(OK);
I like the new notification API!

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1379: for (auto callback :
waiting_for_confirmation_callbacks_)
nit: need a pair of {} here for the for-loop.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.h (right):

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:127: bool
requires_confirmation_;
nit: add a const so we know this variable is not changed after construction.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:179: // Returns an new stream
request which can be used to create a new
nit: s/an/a

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:181: // stream will no be
created until the handshake as been confirmed.
nit: 
s/will no/will not
s/as/has

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:388: void
NotifyAllWaitingForConfirmation(int net_error);
nit: maybe mention "Request" or "StreamRequest" somewhere in the method name so
it's clearer that we are notifying StreamRequests?

e.g. NotifyRequestsOfConfirmation?

[xunjieli, 2017-05-01 20:17:50]

One more nit: QuicStreamRequest is actually another class. I would change the CL
description to mention "QuicChromiumClientSession::StreamRequest" instead of
"QuicStreamRequest" which is a class in quic_stream_factory.h :)

[Ryan Hamilton, 2017-05-01 21:36:28]

Description was changed from

==========
Move the "wait for QUIC handshake confirmation" logic to the QuicStreamRequest
and out of the individual stream subclasses.

This will allow a subsequent CL to remove the OnHandshakeConfirmed upcall.

BUG=716563
==========

to

==========
Move the "wait for QUIC handshake confirmation" logic to the
QuicChromiumClientSession::StreamRequest
and out of the individual stream subclasses.

This will allow a subsequent CL to remove the OnHandshakeConfirmed upcall.

BUG=716563
==========

[Ryan Hamilton, 2017-05-01 21:36:49]

Description was changed from

==========
Move the "wait for QUIC handshake confirmation" logic to the
QuicChromiumClientSession::StreamRequest
and out of the individual stream subclasses.

This will allow a subsequent CL to remove the OnHandshakeConfirmed upcall.

BUG=716563
==========

to

==========
Move the "wait for QUIC handshake confirmation" logic to
QuicChromiumClientSession::StreamRequest and out of the individual stream
subclasses.

This will allow a subsequent CL to remove the OnHandshakeConfirmed upcall.

BUG=716563
==========

[Ryan Hamilton, 2017-05-01 21:42:35]

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc:1198:
&spdy_request_headers_frame_length, &header_stream_offset));
On 2017/05/01 20:15:05, xunjieli wrote:
> Settings header before request headers is much more reasonable. I wonder why
> these tests had the order flipped before..

Ah! Sorry, I meant to call this out in my mail to you.  The initial SETTINGS
frame is sent by the session from OnHandshakeConfirmed, but at the end of the
method. With the old code, a request which was waiting for confirmation would
have OnHandshakeConfirmation() invoked earlier in the method, which would send
out the request *before* the settings. To avoid reentrancy, the new code posts a
tasks to resume the waiting requests which changes the order to what we see
here.

QuicHttpStream, on the other hand, posted a task in the old code, so there is no
behavior change there.

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

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:190: const
base::WeakPtr<QuicChromiumClientSession>& session,
On 2017/05/01 20:15:06, xunjieli wrote:
> Does |session| always outlive the StreamRequest? Will we have a case where
> |session| weak ptr is invalidated? 

The StreamRequest may well outlive the session. The StreamRequest is owned by
the caller, not by the session so it's lifetime can not be bound by the session.

> If not, we could also use a raw QuicChromiumClientSession* ptr here to
simplify
> the lifetime relationship between StreamRequest and the Session. Not
suggesting
> to do it in this CL, but I would like to check to see if this is a viable
> option.

I don't think this is workable, but I'm working on a followup which gets rid of
the WeakPtr in favor of a "Handle", which hides the lifetime issues from the
callers. Hopefully this will be a good end-state for things which need to
interact with the session! *fingers crossed*

https://codereview.chromium.org/2334943002/ (not ready for review, but if you're
curious...)

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:231: if (callback_)
On 2017/05/01 20:15:05, xunjieli wrote:
> I think callback_ is always valid, right? It's never null-ed once set in the
> constructor. Same for line 239 and line 246.

Ah. Good question. No, it's not guaranteed to be valid because these methods do
not follow the "net-style" for async operations. They are invoked even if the
operation returns synchronously. (This is another opportunity for cleanup later!
So many yaks)

Added comments to clarify this.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:290: if
(requires_confirmation_)
On 2017/05/01 20:15:05, xunjieli wrote:
> nit: add curly braces or use early return.

Done.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:299: int rv) {
On 2017/05/01 20:15:05, xunjieli wrote:
> nit: maybe add a DCHECK_NE(ERR_IO_PENDING, rv) here.

Done.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:391:
QuicChromiumClientSession::~QuicChromiumClientSession() {
On 2017/05/01 20:15:06, xunjieli wrote:
> Maybe we can add a DCHECK(waiting_for_confirmation_callbacks_.empty()) here?

Done.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:965:
NotifyAllWaitingForConfirmation(OK);
On 2017/05/01 20:15:05, xunjieli wrote:
> I like the new notification API!

\o/ Thanks!

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1379: for (auto callback :
waiting_for_confirmation_callbacks_)
On 2017/05/01 20:15:06, xunjieli wrote:
> nit: need a pair of {} here for the for-loop.

Hm. I thought this was not needed because the body is a single line, by the same
rule for no {}s in one-line if()s. For example:

void SdchManager::ClearData() {
  blacklisted_domains_.clear();
  allow_latency_experiment_.clear();
  dictionaries_.clear();
  for (auto& observer : observers_)
    observer.OnClearDictionaries();
}

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.h (right):

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:127: bool
requires_confirmation_;
On 2017/05/01 20:15:06, xunjieli wrote:
> nit: add a const so we know this variable is not changed after construction.

Done.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:179: // Returns an new stream
request which can be used to create a new
On 2017/05/01 20:15:06, xunjieli wrote:
> nit: s/an/a

Done.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:181: // stream will no be
created until the handshake as been confirmed.
On 2017/05/01 20:15:06, xunjieli wrote:
> nit: 
> s/will no/will not
> s/as/has

Done.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:388: void
NotifyAllWaitingForConfirmation(int net_error);
On 2017/05/01 20:15:06, xunjieli wrote:
> nit: maybe mention "Request" or "StreamRequest" somewhere in the method name
so
> it's clearer that we are notifying StreamRequests?
> 
> e.g. NotifyRequestsOfConfirmation?

Done.

[Ryan Hamilton, 2017-05-01 21:50:10]

On 2017/05/01 20:17:50, xunjieli wrote:
> One more nit: QuicStreamRequest is actually another class. I would change the
CL
> description to mention "QuicChromiumClientSession::StreamRequest" instead of
> "QuicStreamRequest" which is a class in quic_stream_factory.h :)

Gah! Yes, good point! (More yaks!) Done.

[Ryan Hamilton, 2017-05-01 22:29:22]

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

[commit-bot: I haz the power, 2017-05-01 22:30:02]

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-01 22:39:29]

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

[commit-bot: I haz the power, 2017-05-01 22:39:30]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[Ryan Hamilton, 2017-05-01 23:10:17]

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

[commit-bot: I haz the power, 2017-05-01 23:11:11]

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-02 00:22:06]

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

[commit-bot: I haz the power, 2017-05-02 00:22:07]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-05-02 17:23:05]

lgtm!

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc:1198:
&spdy_request_headers_frame_length, &header_stream_offset));
On 2017/05/01 21:42:32, Ryan Hamilton wrote:
> On 2017/05/01 20:15:05, xunjieli wrote:
> > Settings header before request headers is much more reasonable. I wonder why
> > these tests had the order flipped before..
> 
> Ah! Sorry, I meant to call this out in my mail to you.  The initial SETTINGS
> frame is sent by the session from OnHandshakeConfirmed, but at the end of the
> method. With the old code, a request which was waiting for confirmation would
> have OnHandshakeConfirmation() invoked earlier in the method, which would send
> out the request *before* the settings. To avoid reentrancy, the new code posts
a
> tasks to resume the waiting requests which changes the order to what we see
> here.
> 
> QuicHttpStream, on the other hand, posted a task in the old code, so there is
no
> behavior change there.

Acknowledged. Got it. Thanks for walking me through the code!

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

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:190: const
base::WeakPtr<QuicChromiumClientSession>& session,
On 2017/05/01 21:42:33, Ryan Hamilton wrote:
> On 2017/05/01 20:15:06, xunjieli wrote:
> > Does |session| always outlive the StreamRequest? Will we have a case where
> > |session| weak ptr is invalidated? 
> 
> The StreamRequest may well outlive the session. The StreamRequest is owned by
> the caller, not by the session so it's lifetime can not be bound by the
session.
> 
> > If not, we could also use a raw QuicChromiumClientSession* ptr here to
> simplify
> > the lifetime relationship between StreamRequest and the Session. Not
> suggesting
> > to do it in this CL, but I would like to check to see if this is a viable
> > option.
> 
> I don't think this is workable, but I'm working on a followup which gets rid
of
> the WeakPtr in favor of a "Handle", which hides the lifetime issues from the
> callers. Hopefully this will be a good end-state for things which need to
> interact with the session! *fingers crossed*
> 
> https://codereview.chromium.org/2334943002/ (not ready for review, but if
you're
> curious...)

Got it. That makes sense. The "Handle" approach is neat! I like the idea that
the Handle is guaranteed to be valid throughout.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:231: if (callback_)
On 2017/05/01 21:42:35, Ryan Hamilton wrote:
> On 2017/05/01 20:15:05, xunjieli wrote:
> > I think callback_ is always valid, right? It's never null-ed once set in the
> > constructor. Same for line 239 and line 246.
> 
> Ah. Good question. No, it's not guaranteed to be valid because these methods
do
> not follow the "net-style" for async operations. They are invoked even if the
> operation returns synchronously. (This is another opportunity for cleanup
later!
> So many yaks)
> 
> Added comments to clarify this.

Ah, that's subtle. Thanks for the comment.

https://codereview.chromium.org/2848923004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1379: for (auto callback :
waiting_for_confirmation_callbacks_)
On 2017/05/01 21:42:32, Ryan Hamilton wrote:
> On 2017/05/01 20:15:06, xunjieli wrote:
> > nit: need a pair of {} here for the for-loop.
> 
> Hm. I thought this was not needed because the body is a single line, by the
same
> rule for no {}s in one-line if()s. For example:
> 
> void SdchManager::ClearData() {
>   blacklisted_domains_.clear();
>   allow_latency_experiment_.clear();
>   dictionaries_.clear();
>   for (auto& observer : observers_)
>     observer.OnClearDictionaries();
> }

Got it. I didn't know that braces are optional for for-loops.
I doublechecked, and you are right. 
https://google.github.io/styleguide/cppguide.html says:
"Braces are optional for single-statement loops."

[Ryan Hamilton, 2017-05-02 17:28:07]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2017-05-02 17:28:32]

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-02 17:33:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493746087624910,
"parent_rev": "99df95e150d6ac9b9c7e7a85a9261d609ae53b71", "commit_rev":
"d606b6c39dbce82e048ed0a737d56dc7d8b0ea32"}

[commit-bot: I haz the power, 2017-05-02 17:34:01]

Description was changed from

==========
Move the "wait for QUIC handshake confirmation" logic to
QuicChromiumClientSession::StreamRequest and out of the individual stream
subclasses.

This will allow a subsequent CL to remove the OnHandshakeConfirmed upcall.

BUG=716563
==========

to

==========
Move the "wait for QUIC handshake confirmation" logic to
QuicChromiumClientSession::StreamRequest and out of the individual stream
subclasses.

This will allow a subsequent CL to remove the OnHandshakeConfirmed upcall.

BUG=716563

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

[commit-bot: I haz the power, 2017-05-02 17:34:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d606b6c39dbce82e048ed0a737d5...