Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory

Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory to QuicHttpStream and BidirectionalStreamQuicImpl.

BUG=611820
Committed: https://crrev.com/cb1d6bdee0b6971ae77243cf78668e3973265a7a
Cr-Commit-Position: refs/heads/master@{#400551}

[Ryan Hamilton, 2016-06-16 22:17:26]

Description was changed from

==========
Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory
to QuicHttpStream.
==========

to

==========
Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory to
QuicHttpStream.

BUG=611820
==========

[Ryan Hamilton, 2016-06-16 22:28:13]

Description was changed from

==========
Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory to
QuicHttpStream.

BUG=611820
==========

to

==========
Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory to
QuicHttpStream and BidirectionalStreamQuicImpl.

BUG=611820
==========

[Ryan Hamilton, 2016-06-16 22:33:13]

rch@chromium.org changed reviewers:
+ ian@glaxstar.com, rtenneti@chromium.org

[Ryan Hamilton, 2016-06-16 22:33:14]

rtenneti: PTAL
xunjieli: Please review BidirectionalStreamQuicImpl
ianswett: FYI

[Ryan Hamilton, 2016-06-16 22:35:55]

rch@chromium.org changed reviewers:
+ xunjieli@chromium.org
- ian@glaxstar.com

[xunjieli, 2016-06-16 22:52:13]

I will try testing this manually tomorrow. Two quick comments.

https://codereview.chromium.org/2077683002/diff/40001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2077683002/diff/40001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl.cc:302: if (!was_handshake_confirmed_ &&
request_info_->method == "POST") {
If |was_handshake_confirmed_| is false and method != POST, will we ever call
StartStream() twice? It looks like it might.. If so then that will be bad since
we aren't supposed to call delegate_->OnStreamReady() twice.

https://codereview.chromium.org/2077683002/diff/40001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2077683002/diff/40001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl.h:73: void StartStream();
Can you add a brief comment? Since this method is invoked in OnStreamReady()
where the stream is already started, it is a bit odd to call it StartStream.

[ramant (doing other things), 2016-06-17 00:39:55]

Hi Ryan,
  Didn't look at bidirectional stream changes. Other QUIC changes LGTM.

[xunjieli, 2016-06-17 01:31:46]

LGTM mod the comment on adding a documentation to StartStream().

https://codereview.chromium.org/2077683002/diff/40001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2077683002/diff/40001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl.cc:302: if (!was_handshake_confirmed_ &&
request_info_->method == "POST") {
On 2016/06/16 22:52:13, xunjieli wrote:
> If |was_handshake_confirmed_| is false and method != POST, will we ever call
> StartStream() twice? It looks like it might.. If so then that will be bad
since
> we aren't supposed to call delegate_->OnStreamReady() twice.

Ah, nvm. It won't happen. I was reading the code wrong.

[xunjieli, 2016-06-17 13:40:37]

I don't know much about QUIC's test fixtures. I suppose it won't be easy to add
a unit test? A unit test would be nice, so we can avoid regression. Is there
some way to delay  session_->GetCryptoStream()->CryptoConnect() so we can test
the case where waiting_for_confirmation_ = true?

one more nit below.

https://codereview.chromium.org/2077683002/diff/60001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/2077683002/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory_test.cc:614: // Posts require handshake
confirmation, so this will return asynchronously.
The comment is now outdated.

[Ian Swett, 2016-06-17 13:49:08]

ianswett@google.com changed reviewers:
+ ianswett@google.com

[Ian Swett, 2016-06-17 13:49:09]

lgtm

It'd be nice to have bit more testing to ensure this doesn't break again, but I
don't have any concrete suggestions.

[Ryan Hamilton, 2016-06-17 17:37:22]

BIDI tests added. Woo hoo!

https://codereview.chromium.org/2077683002/diff/60001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/2077683002/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory_test.cc:614: // Posts require handshake
confirmation, so this will return asynchronously.
On 2016/06/17 13:40:37, xunjieli wrote:
> The comment is now outdated.

Done.

[xunjieli, 2016-06-17 18:05:43]

Looks great! Thanks for adding the test. A couple of minor suggestions.

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl.h:73: void StartStream();
We could rename it to NotifyStreamReady() to be more explicit about the fact
that we are about to notify the delegate. If we choose to name it
NotifyStreamReady(), we can move this method down before or after NotifyError().

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:84: is_ready_ = true;
nit: maybe set |is_ready_| before exiting the |loop_|? 

is_ready_ = true;
loop_->Quit();

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:730:
delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
Does QuicStreamRequest always finish synchronously? If it does, maybe worth
adding a comment here. If it doesn't, maybe add a 
base::RunLoop().RunUntilIdle() to flush posted tasks from the message loop?

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:1545: ConfirmHandshake();
You have examples here to demonstrate that GET doesn't require handshake
confirmation before sending the headers. 
Could you make a new unit test where we exercise PUT? You can duplicate the code
from BidirectionalStreamQuicImplTest.PostRequest, and change the method to PUT.
We won't gain much from having that test, but it serves as an example that PUT
also does not require handshake confirmation in 0RTT.

[Ryan Hamilton, 2016-06-17 18:20:35]

Thanks!

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl.h:73: void StartStream();
On 2016/06/17 18:05:42, xunjieli wrote:
> We could rename it to NotifyStreamReady() to be more explicit about the fact
> that we are about to notify the delegate. If we choose to name it
> NotifyStreamReady(), we can move this method down before or after
NotifyError().

Done.

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:84: is_ready_ = true;
On 2016/06/17 18:05:42, xunjieli wrote:
> nit: maybe set |is_ready_| before exiting the |loop_|? 
> 
> is_ready_ = true;
> loop_->Quit();

Done.

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:730:
delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
On 2016/06/17 18:05:42, xunjieli wrote:
> Does QuicStreamRequest always finish synchronously? If it does, maybe worth
> adding a comment here. If it doesn't, maybe add a 
> base::RunLoop().RunUntilIdle() to flush posted tasks from the message loop?

No, it's not guaranteed to complete synchronously. For example, if there are max
streams open, it will not complete until the next stream closes. Or if handshake
confirmation is required, it won't return until the SHLO is received. I'm not
sure that adding RunUntilIdle() is a good idea though.

https://codereview.chromium.org/2077683002/diff/80001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl_unittest.cc:1545: ConfirmHandshake();
On 2016/06/17 18:05:43, xunjieli wrote:
> You have examples here to demonstrate that GET doesn't require handshake
> confirmation before sending the headers. 
> Could you make a new unit test where we exercise PUT? You can duplicate the
code
> from BidirectionalStreamQuicImplTest.PostRequest, and change the method to
PUT.
> We won't gain much from having that test, but it serves as an example that PUT
> also does not require handshake confirmation in 0RTT.

Done.

[xunjieli, 2016-06-17 18:32:14]

https://codereview.chromium.org/2077683002/diff/100001/net/quic/bidirectional...
File net/quic/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/2077683002/diff/100001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl_unittest.cc:1622: ConfirmHandshake();
I am a bit confused. If the handshake confirmation is not required for GET in
the case of 0-RTT, can we omit ConfirmHandshake() here and for those tests where
GET is used?

[Ryan Hamilton, 2016-06-17 18:51:37]

https://codereview.chromium.org/2077683002/diff/100001/net/quic/bidirectional...
File net/quic/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/2077683002/diff/100001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl_unittest.cc:1622: ConfirmHandshake();
On 2016/06/17 18:32:13, xunjieli wrote:
> I am a bit confused. If the handshake confirmation is not required for GET in
> the case of 0-RTT, can we omit ConfirmHandshake() here and for those tests
where
> GET is used?

Ah, I was overly aggressive here. Yes, this can be removed. Done. However, there
are two other GET tests: ServerSendsRstAfterReadData and
ServerSendsRstAfterHeaders which fail if the handshake has not been confirmed
because in that case the error is QUIC_HANDSHAKE_FAILED not QUIC_PROTOCOL_ERROR.
But since it's not possible to actually get a RST_STREAM frame unless the
handshake has been confirmed, actually confirming the handshake makes the most
sense.

[Ryan Hamilton, 2016-06-17 18:52:05]

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

[commit-bot: I haz the power, 2016-06-17 18:52:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2077683002/120001

[xunjieli, 2016-06-17 18:54:02]

lgtm

https://codereview.chromium.org/2077683002/diff/100001/net/quic/bidirectional...
File net/quic/bidirectional_stream_quic_impl_unittest.cc (right):

https://codereview.chromium.org/2077683002/diff/100001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl_unittest.cc:1622: ConfirmHandshake();
On 2016/06/17 18:51:36, Ryan Hamilton wrote:
> On 2016/06/17 18:32:13, xunjieli wrote:
> > I am a bit confused. If the handshake confirmation is not required for GET
in
> > the case of 0-RTT, can we omit ConfirmHandshake() here and for those tests
> where
> > GET is used?
> 
> Ah, I was overly aggressive here. Yes, this can be removed. Done. However,
there
> are two other GET tests: ServerSendsRstAfterReadData and
> ServerSendsRstAfterHeaders which fail if the handshake has not been confirmed
> because in that case the error is QUIC_HANDSHAKE_FAILED not
QUIC_PROTOCOL_ERROR.
> But since it's not possible to actually get a RST_STREAM frame unless the
> handshake has been confirmed, actually confirming the handshake makes the most
> sense.

That makes sense. Thanks for the clarification!

[commit-bot: I haz the power, 2016-06-17 20:04:10]

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

[commit-bot: I haz the power, 2016-06-17 20:04:11]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2016-06-18 00:26:42]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-06-18 00:26:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org, ianswett@google.com
Link to the patchset: https://codereview.chromium.org/2077683002/#ps120001
(title: "tweak")

[commit-bot: I haz the power, 2016-06-18 00:27:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2077683002/120001

[commit-bot: I haz the power, 2016-06-18 00:33:41]

Description was changed from

==========
Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory to
QuicHttpStream and BidirectionalStreamQuicImpl.

BUG=611820
==========

to

==========
Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory to
QuicHttpStream and BidirectionalStreamQuicImpl.

BUG=611820
==========

[commit-bot: I haz the power, 2016-06-18 00:33:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-18 00:35:08]

Description was changed from

==========
Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory to
QuicHttpStream and BidirectionalStreamQuicImpl.

BUG=611820
==========

to

==========
Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory to
QuicHttpStream and BidirectionalStreamQuicImpl.

BUG=611820

Committed: https://crrev.com/cb1d6bdee0b6971ae77243cf78668e3973265a7a
Cr-Commit-Position: refs/heads/master@{#400551}
==========

[commit-bot: I haz the power, 2016-06-18 00:35:09]

Patchset 7 (id:??) landed as
https://crrev.com/cb1d6bdee0b6971ae77243cf78668e3973265a7a
Cr-Commit-Position: refs/heads/master@{#400551}