Change the way BidirectionalStreamQuicImpl detects that a stream/session

Change the way BidirectionalStreamQuicImpl detects that a stream/session
was closed abnormally.

Currently BidirectionalStreamQuicImpl::OnClose() does not invoke
NotifyError if the stream and connection error codes are both
NO_ERROR. However, this is not guaranteeded to be true when a stream
shuts down normally. For example, if the connection is closed with
QUIC_NO_ERROR (which is not expected behavior, but is possible) then
NotifyError will not be invoked. Now, it turns out that the current code
handles this case, perhaps inadvertantly, by invoking NotifyError
in OnSessionClosed.

As part of crbug.com/716563, I'm attempting to remove the upcalls from
the QUIC layer to the HTTP layer, so I will be removing OnSessionClosed.
In order to handle the NO_ERROR case, we need a better way to detect
that a stream was closed normally. Thankfully, we can do that by
checking that a fin was sent and a fin was received.

This change also clears session_ in ResetStream which ensures that 
OnSessionClosed is not invoked after OnClose is called (and makes it
more obvious that the follow-up CL to remove OnSessionClosed is safe).

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

[Ryan Hamilton, 2017-05-01 18:14:11]

Description was changed from

==========
Change the way BidirectionalStreamQuicImpl detects that a stream/session
was closed abnormally.

Currently BidirectionalStreamQuicImpl::OnClose() does not invoke
NotifyError if the stream and connection error codes are both
NO_ERROR. However, this is not guaranteeded to be true when a stream
shuts down normally. For example, if the connection is closed with
QUIC_NO_ERROR (which is not expected behavior, but is possible) then
NotifyError will not be invoked. Now, it turns out that the current code
handles this case, perhaps inadvertantly, by invoking NotifyError
in OnSessionClosed.

As part of crbug.com/716563, I'm attempting to remove the upcalls from
the QUIC layer to the HTTP layer, so I will be removing OnSessionClosed.
In order to handle the NO_ERROR case, we need a better way to detect
that a stream was closed normally. Thankfully, we can do that by
checking that a fin was sent and a fin was received.

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

to

==========
Change the way BidirectionalStreamQuicImpl detects that a stream/session
was closed abnormally.

Currently BidirectionalStreamQuicImpl::OnClose() does not invoke
NotifyError if the stream and connection error codes are both
NO_ERROR. However, this is not guaranteeded to be true when a stream
shuts down normally. For example, if the connection is closed with
QUIC_NO_ERROR (which is not expected behavior, but is possible) then
NotifyError will not be invoked. Now, it turns out that the current code
handles this case, perhaps inadvertantly, by invoking NotifyError
in OnSessionClosed.

As part of crbug.com/716563, I'm attempting to remove the upcalls from
the QUIC layer to the HTTP layer, so I will be removing OnSessionClosed.
In order to handle the NO_ERROR case, we need a better way to detect
that a stream was closed normally. Thankfully, we can do that by
checking that a fin was sent and a fin was received.

This change also clears session_ in ResetStream which ensures that 
OnSessionClosed is not invoked after OnClose is called (and makes it
more obvious that the follow-up CL to remove OnSessionClosed is safe).

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

[Ryan Hamilton, 2017-05-01 18:14:25]

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

[xunjieli, 2017-05-01 20:22:02]

lgtm with one question below. The change makes sense to me. Thank you for the
detailed CL description and background info!

https://codereview.chromium.org/2851293002/diff/1/net/quic/chromium/bidirecti...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2851293002/diff/1/net/quic/chromium/bidirecti...
net/quic/chromium/bidirectional_stream_quic_impl.cc:289: // the delegate.
nice commenting!

https://codereview.chromium.org/2851293002/diff/1/net/quic/chromium/bidirecti...
net/quic/chromium/bidirectional_stream_quic_impl.cc:364: if (session_) {
Same question here: if Session guarantees to outlive BidiStream after you
eliminate re-entrancy, maybe we could use a raw ptr for |session_| to make the
lifetime relationship between session and bidistream simpler?

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

Thanks!

https://codereview.chromium.org/2851293002/diff/1/net/quic/chromium/bidirecti...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2851293002/diff/1/net/quic/chromium/bidirecti...
net/quic/chromium/bidirectional_stream_quic_impl.cc:289: // the delegate.
On 2017/05/01 20:22:02, xunjieli wrote:
> nice commenting!

Thanks!

https://codereview.chromium.org/2851293002/diff/1/net/quic/chromium/bidirecti...
net/quic/chromium/bidirectional_stream_quic_impl.cc:364: if (session_) {
On 2017/05/01 20:22:01, xunjieli wrote:
> Same question here: if Session guarantees to outlive BidiStream after you
> eliminate re-entrancy, maybe we could use a raw ptr for |session_| to make the
> lifetime relationship between session and bidistream simpler?

I believe that, like QuicHttpStream, the BidiStream is owned by the upper-layer,
not by the session and is not guaranteed to outlive the session. This detail
will be hidden in the Handle CL, thankfully. 

I did spend a good deal of time investigating what it would take to keep the
sessions until there are no more "things" which want it (streams, factory, etc).
It's possible, but I think it might actually be worse. If we switch to a,
basically, reference counted implementation then it's possible to "leak"
sessions if we're not careful. To solve this, we could have an explicit deletion
path which, say, the Factory could invoke when it's under pressure. But then
we're back to not having the lifetime guaranteed.

I *think* the Handle approach is a good compromise. The callers will have a
Handle, which they own, that will serve as a facade to the session. If the
session goes away, the Handle will still be valid and will, for example, return
ERR_CONNECTION_CLOSED (or some such) if the caller tries to write to the Handle.
I think I'm about a day or two away (once this current CL lands) from having
that follow up ready for review.

*fingers crossed*

[Ryan Hamilton, 2017-05-01 23:25:07]

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:26:12]

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:33:56]

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

[commit-bot: I haz the power, 2017-05-02 00:33:57]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-05-02 17:32:55]

still lgtm

https://codereview.chromium.org/2851293002/diff/1/net/quic/chromium/bidirecti...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2851293002/diff/1/net/quic/chromium/bidirecti...
net/quic/chromium/bidirectional_stream_quic_impl.cc:364: if (session_) {
On 2017/05/01 21:49:23, Ryan Hamilton wrote:
> On 2017/05/01 20:22:01, xunjieli wrote:
> > Same question here: if Session guarantees to outlive BidiStream after you
> > eliminate re-entrancy, maybe we could use a raw ptr for |session_| to make
the
> > lifetime relationship between session and bidistream simpler?
> 
> I believe that, like QuicHttpStream, the BidiStream is owned by the
upper-layer,
> not by the session and is not guaranteed to outlive the session. This detail
> will be hidden in the Handle CL, thankfully. 
> 
> I did spend a good deal of time investigating what it would take to keep the
> sessions until there are no more "things" which want it (streams, factory,
etc).
> It's possible, but I think it might actually be worse. If we switch to a,
> basically, reference counted implementation then it's possible to "leak"
> sessions if we're not careful. To solve this, we could have an explicit
deletion
> path which, say, the Factory could invoke when it's under pressure. But then
> we're back to not having the lifetime guaranteed.
> 
> I *think* the Handle approach is a good compromise. The callers will have a
> Handle, which they own, that will serve as a facade to the session. If the
> session goes away, the Handle will still be valid and will, for example,
return
> ERR_CONNECTION_CLOSED (or some such) if the caller tries to write to the
Handle.
> I think I'm about a day or two away (once this current CL lands) from having
> that follow up ready for review.
> 
> *fingers crossed*

Acknowledged. Ah, I see. CL description mentioned that OnSessionClosed will be
gone, so I thought we'd have the session outlive all requests/streams. The
"Handle" part is the missing piece. I agree that it's a cleaner approach for the
lifetime issues.

[Ryan Hamilton, 2017-05-02 18:12:25]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2017-05-02 18:12:58]

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 18:43:06]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493748745212470,
"parent_rev": "2fd356a58dfbc3e653f2c4eb3f781cb6a88c3472", "commit_rev":
"5bc140b118c615e92e0393d8baf846d1a9bcbdba"}

[commit-bot: I haz the power, 2017-05-02 18:44:07]

Description was changed from

==========
Change the way BidirectionalStreamQuicImpl detects that a stream/session
was closed abnormally.

Currently BidirectionalStreamQuicImpl::OnClose() does not invoke
NotifyError if the stream and connection error codes are both
NO_ERROR. However, this is not guaranteeded to be true when a stream
shuts down normally. For example, if the connection is closed with
QUIC_NO_ERROR (which is not expected behavior, but is possible) then
NotifyError will not be invoked. Now, it turns out that the current code
handles this case, perhaps inadvertantly, by invoking NotifyError
in OnSessionClosed.

As part of crbug.com/716563, I'm attempting to remove the upcalls from
the QUIC layer to the HTTP layer, so I will be removing OnSessionClosed.
In order to handle the NO_ERROR case, we need a better way to detect
that a stream was closed normally. Thankfully, we can do that by
checking that a fin was sent and a fin was received.

This change also clears session_ in ResetStream which ensures that 
OnSessionClosed is not invoked after OnClose is called (and makes it
more obvious that the follow-up CL to remove OnSessionClosed is safe).

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

to

==========
Change the way BidirectionalStreamQuicImpl detects that a stream/session
was closed abnormally.

Currently BidirectionalStreamQuicImpl::OnClose() does not invoke
NotifyError if the stream and connection error codes are both
NO_ERROR. However, this is not guaranteeded to be true when a stream
shuts down normally. For example, if the connection is closed with
QUIC_NO_ERROR (which is not expected behavior, but is possible) then
NotifyError will not be invoked. Now, it turns out that the current code
handles this case, perhaps inadvertantly, by invoking NotifyError
in OnSessionClosed.

As part of crbug.com/716563, I'm attempting to remove the upcalls from
the QUIC layer to the HTTP layer, so I will be removing OnSessionClosed.
In order to handle the NO_ERROR case, we need a better way to detect
that a stream was closed normally. Thankfully, we can do that by
checking that a fin was sent and a fin was received.

This change also clears session_ in ResetStream which ensures that 
OnSessionClosed is not invoked after OnClose is called (and makes it
more obvious that the follow-up CL to remove OnSessionClosed is safe).

BUG=716563

Review-Url: https://codereview.chromium.org/2851293002
Cr-Commit-Position: refs/heads/master@{#468714}
Committed:
https://chromium.googlesource.com/chromium/src/+/5bc140b118c615e92e0393d8baf8...
==========

[commit-bot: I haz the power, 2017-05-02 18:44:08]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5bc140b118c615e92e0393d8baf8...