Add a new QuicChromiumClientSession::Handle class

The Handle is owned by the holder of the Handle and can outlive the session. In the event that it does outlive the session, it will record state from the session in member variables so that the holder of a session need not be aware of the lifetime of the underlying session.

As a consequence, this change eliminates the need for the QuicChromiumClientSession::Observer interface, and hence eliminates the upcalls from the session to the streams. There are still upcalls from the QuicStream to the QuicHttpStream (and friends) but those can be removed in subsequent CLs by following the same pattern.

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

[Ryan Hamilton, 2016-09-12 23:04:09]

Description was changed from

==========
Add a new QuicChromiumClientSession::Handle class to isolate the
QuicHttpStream from the underlying session. In particular, this
prevents the QuicHttpStream from closing the connection
or doing anything otherwise problematic with it.

In addition, the Handle is owned by the QuicHttpStream which
simplifies the ownership model slightly.
==========

to

==========
Add a new QuicChromiumClientSession::Handle class to isolate the
QuicHttpStream from the underlying session. In particular, this
prevents the QuicHttpStream from closing the connection
or doing anything otherwise problematic with it.

In addition, the Handle is owned by the QuicHttpStream which
simplifies the ownership model slightly.
==========

[Ryan Hamilton, 2016-09-12 23:27:08]

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

[commit-bot: I haz the power, 2016-09-12 23:27:34]

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, 2016-09-12 23:30:42]

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

[commit-bot: I haz the power, 2016-09-12 23:30:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ryan Hamilton, 2017-05-01 18:44:55]

Description was changed from

==========
Add a new QuicChromiumClientSession::Handle class to isolate the
QuicHttpStream from the underlying session. In particular, this
prevents the QuicHttpStream from closing the connection
or doing anything otherwise problematic with it.

In addition, the Handle is owned by the QuicHttpStream which
simplifies the ownership model slightly.
==========

to

==========
The Handle is owned by the holder of the Handle and can outlive the session. In
the event that it does outlive the session, it will record state from the
session in member variables so that the holder of a session need not be aware of
the lifetime of the underlying session.

As a consequence, this change eliminates the need for the
QuicChromiumClientSession::Observer interface, and hence eliminates the upcalls
from the session to the streams. There are still upcalls from the QuicStream to
the QuicHttpStream (and friends) but those can be removed in subsequent CLs by
following the same pattern.

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

[Ryan Hamilton, 2017-05-02 23:01:14]

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

[Ryan Hamilton, 2017-05-02 23:01:15]

I think this is ready for review. But I confess I've been staring at it for so
long that it's becoming a bit of a blur :)

[xunjieli, 2017-05-03 21:17:33]

On 2017/05/02 23:01:15, Ryan Hamilton wrote:
> I think this is ready for review. But I confess I've been staring at it for so
> long that it's becoming a bit of a blur :)

I feel the same when working on a CL for too long :) 
Sorry for the delay -- I was distracted today by other things. I will get to
this by tomorrow morning.

[xunjieli, 2017-05-04 16:54:43]

Sorry for the delay. Here's my first round of review to get the process started
:)

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:71: int rv =
stream_request_->StartRequest(base::Bind(
Instead of requiring BidiStream/HttpStream to know about both StreamRequest and
Handle, have you considered eliminating one of these two from a consumer point
of view?

For example, we can make StreamRequest a nested class within Handle.
Handle::Start() calls StreamRequest::StartRequest(). Then BidiStream/HttpStream
doesn't need to know about StreamRequest.

If StreamRequest::StartRequest() finishes with an error that leads to the
Session being synchronously destroyed, we can handle that in the Handle class.

Right now Handle and StreamRequest have references to each other. This
cross-reference takes some effort to understand.

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:187: int rv =
stream_->WritevStreamData(
Potential crash?
With OnSessionClosed() removed, if the |session| is gone synchronously because
SendRequestHeaders() errors out synchronously, we won't know |stream_| is
invalid.

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.h:79:
QuicChromiumClientSession::Handle session_;
nit: can this be a const member?

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

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1554: return
*self_handle_.get();
So this makes a new Handle?  This is quite subtle.
I suggest we make the creation of a new Handle more explicit.
We could get rid of |self_handle_|, and explicitly create a new one each time.

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

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:74: class NET_EXPORT_PRIVATE
Handle : public MultiplexedSessionHandle {
Can we disallow copy and assign for this class so know where instances of Handle
are created?

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:84: // the handle is not valid.
nit: delete the second line.

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:89: // stream will not be
created until the handshake as been confirmed.
nit: s/as/has

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:161: // Information latched
from the session when it is closed.
nit: consider rephrasing "latched" to "saved" or something more general.

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
File net/quic/chromium/quic_stream_factory.h (right):

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_stream_factory.h:138: void SetSession(const
QuicChromiumClientSession::Handle& session);
I think for simplicity we should keep this as QuicChromiumClientSession,
otherwise the concept of a handle gets rather complicated. Each
BidiStream/HttpStream holds a Handle to the session, but each
QuicStreamFactory::QuicStreamRequest can also take a copy of
QuicChromiumClientSession's self handle. The Handle of
QuicStreamFactory::QuicStreamRequest is also not passed to
BidiStream/HttpStream.

[Ryan Hamilton, 2017-05-04 18:45:53]

Thanks for the great review. I'm definitely looking forward to dig in, though
before I start making changes I'd love to sync up and get on the same page about
a couple design points. Let's chat?

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:71: int rv =
stream_request_->StartRequest(base::Bind(
On 2017/05/04 16:54:42, xunjieli wrote:
> Instead of requiring BidiStream/HttpStream to know about both StreamRequest
and
> Handle, have you considered eliminating one of these two from a consumer point
> of view?
> 
> For example, we can make StreamRequest a nested class within Handle.
> Handle::Start() calls StreamRequest::StartRequest(). Then
BidiStream/HttpStream
> doesn't need to know about StreamRequest.
> 
> If StreamRequest::StartRequest() finishes with an error that leads to the
> Session being synchronously destroyed, we can handle that in the Handle class.
> 
> Right now Handle and StreamRequest have references to each other. This
> cross-reference takes some effort to understand.

I hear ya! I spent a bunch of time thinking about this because, as you say, it
seems like hiding the Request in the Handle makes sense. However, I think it
complicates the Handle a bit, because I think it makes copy semantics
problematic. If I have a handle and start a request and then make a copy of the
handle do they both have the same request? That can't work. Do they get
different requests? That also sounds problematic.

So one solution would be to make the Handle non-copyable, but I'm not in love
with that approach either 'cause I think it makes life a bit harder on the
caller.  

Long story short, I'm inclined to leave it as-is, but happy to revisit if you
feel strongly.

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
File net/quic/chromium/quic_stream_factory.h (right):

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_stream_factory.h:138: void SetSession(const
QuicChromiumClientSession::Handle& session);
On 2017/05/04 16:54:42, xunjieli wrote:
> I think for simplicity we should keep this as QuicChromiumClientSession,
> otherwise the concept of a handle gets rather complicated. Each
> BidiStream/HttpStream holds a Handle to the session, but each
> QuicStreamFactory::QuicStreamRequest can also take a copy of
> QuicChromiumClientSession's self handle. The Handle of
> QuicStreamFactory::QuicStreamRequest is also not passed to
> BidiStream/HttpStream. 

Hm. I'm not sure I follow you. My vision (perhaps poorly thought out) is that
everything that needs to talk to a session (with the exception of the
QuicStreamFactory which owns the session) should use a Handle to it.

Perhaps we can chat about this design?

[xunjieli, 2017-05-04 19:17:12]

> Hm. I'm not sure I follow you. My vision (perhaps poorly thought out) is that
> everything that needs to talk to a session (with the exception of the
> QuicStreamFactory which owns the session) should use a Handle to it.
> 
> Perhaps we can chat about this design?

Yes, let's chat more about this design. What I had in mind is more like a
ClientSocketHandle where it represents both the connection and stream (basically
the thing that a consumer needs to send/receive packets)

With QuicChromiumClientSession gone, we still have a raw pointer to
QuicChromiumClientStream, which is owned by the Session. The raw
QuicChromiumClientStream pointer is dangerous because QuicChromiumClientSession
can be gone at any point even though that it is now safely hidden behind the new
Handle interface. If we use a Handle to hide both QuicChromiumClientStream and
QuicChromiumClientSession, then we should make it non-copy-able but only
movable.

[Ryan Hamilton, 2017-05-05 03:50:25]

PTAL

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:71: int rv =
stream_request_->StartRequest(base::Bind(
On 2017/05/04 18:45:53, Ryan Hamilton wrote:
> On 2017/05/04 16:54:42, xunjieli wrote:
> > Instead of requiring BidiStream/HttpStream to know about both StreamRequest
> and
> > Handle, have you considered eliminating one of these two from a consumer
point
> > of view?
> > 
> > For example, we can make StreamRequest a nested class within Handle.
> > Handle::Start() calls StreamRequest::StartRequest(). Then
> BidiStream/HttpStream
> > doesn't need to know about StreamRequest.
> > 
> > If StreamRequest::StartRequest() finishes with an error that leads to the
> > Session being synchronously destroyed, we can handle that in the Handle
class.
> > 
> > Right now Handle and StreamRequest have references to each other. This
> > cross-reference takes some effort to understand.
> 
> I hear ya! I spent a bunch of time thinking about this because, as you say, it
> seems like hiding the Request in the Handle makes sense. However, I think it
> complicates the Handle a bit, because I think it makes copy semantics
> problematic. If I have a handle and start a request and then make a copy of
the
> handle do they both have the same request? That can't work. Do they get
> different requests? That also sounds problematic.
> 
> So one solution would be to make the Handle non-copyable, but I'm not in love
> with that approach either 'cause I think it makes life a bit harder on the
> caller.  
> 
> Long story short, I'm inclined to leave it as-is, but happy to revisit if you
> feel strongly.

Now that we decided to make the handle non-copyable this is a non-issue, so I've
made this change. It's a nice cleanup! thanks

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:187: int rv =
stream_->WritevStreamData(
On 2017/05/04 16:54:42, xunjieli wrote:
> Potential crash?
> With OnSessionClosed() removed, if the |session| is gone synchronously because
> SendRequestHeaders() errors out synchronously, we won't know |stream_| is
> invalid.

Seems like a potential crash, but as discussed, it's pre-existing.

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.h:79:
QuicChromiumClientSession::Handle session_;
On 2017/05/04 16:54:42, xunjieli wrote:
> nit: can this be a const member?

If it were const then I couldn't call non-const methods on it, right?

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

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:1554: return
*self_handle_.get();
On 2017/05/04 16:54:42, xunjieli wrote:
> So this makes a new Handle?  This is quite subtle.
> I suggest we make the creation of a new Handle more explicit.
> We could get rid of |self_handle_|, and explicitly create a new one each time.

Done.

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

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:74: class NET_EXPORT_PRIVATE
Handle : public MultiplexedSessionHandle {
On 2017/05/04 16:54:42, xunjieli wrote:
> Can we disallow copy and assign for this class so know where instances of
Handle
> are created?

Done.

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:84: // the handle is not valid.
On 2017/05/04 16:54:42, xunjieli wrote:
> nit: delete the second line.

Done.

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:89: // stream will not be
created until the handshake as been confirmed.
On 2017/05/04 16:54:42, xunjieli wrote:
> nit: s/as/has

Removed this method completely. Yay! :)

https://codereview.chromium.org/2334943002/diff/140001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:161: // Information latched
from the session when it is closed.
On 2017/05/04 16:54:42, xunjieli wrote:
> nit: consider rephrasing "latched" to "saved" or something more general.

Heh, sure. "latched" is pretty common vocabulary in GFE land, but this isn't GFE
code :)

[Ryan Hamilton, 2017-05-05 03:53:23]

On 2017/05/04 19:17:12, xunjieli wrote:
> > Hm. I'm not sure I follow you. My vision (perhaps poorly thought out) is
that
> > everything that needs to talk to a session (with the exception of the
> > QuicStreamFactory which owns the session) should use a Handle to it.
> > 
> > Perhaps we can chat about this design?
> 
> Yes, let's chat more about this design. What I had in mind is more like a
> ClientSocketHandle where it represents both the connection and stream
(basically
> the thing that a consumer needs to send/receive packets)
> 
> With QuicChromiumClientSession gone, we still have a raw pointer to
> QuicChromiumClientStream, which is owned by the Session. The raw
> QuicChromiumClientStream pointer is dangerous because
QuicChromiumClientSession
> can be gone at any point even though that it is now safely hidden behind the
new
> Handle interface. If we use a Handle to hide both QuicChromiumClientStream and
> QuicChromiumClientSession, then we should make it non-copy-able but only
> movable.

As discussed offline, that's kinda tricky because the caller needs to be created
before an underlying stream is allocated. The long term vision we discussed is
that there will be 2 handles, a session and an stream. Both of which will have
async method which take Callbacks and neither will use upcalls. This CL only
implements the first. I'm working on the second (though it's going to be several
CLs to get there, I think)

[xunjieli, 2017-05-05 14:10:01]

Looks great, I am very close to signing off, just a couple of things below.

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.h:78:
std::unique_ptr<QuicChromiumClientSession::Handle> session_;
I guess this can be "const std::unique_ptr<>" ?

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

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:299: new
StreamRequest(session_->CreateHandle(), requires_confirmation));
Have you considered the alternative approach below?

Make StreamRequest as an inner class of the session Handle, so it can access
Handles members. That way we can avoid creating another Handle here and the
forward declaration of StreamRequest in the header file.

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

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:365: // Return a Handle to this
session.
minor nit: s/Return/Returns

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
File net/quic/chromium/quic_http_stream.h (right):

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
net/quic/chromium/quic_http_stream.h:145: const
QuicChromiumClientSession::Handle* quic_session() const {
We have two versions of accessors? Can we get rid of one?

C++ style guide discourages function overloads
https://google.github.io/styleguide/cppguide.html#Function_Overloading

"If you want to overload a function, consider qualifying the name with some
information about the arguments, e.g., AppendString(), AppendInt() rather than
just Append(). "

[xunjieli, 2017-05-05 14:10:10]

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

[commit-bot: I haz the power, 2017-05-05 14:10:30]

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-05 15:58:46]

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

[commit-bot: I haz the power, 2017-05-05 15:58:47]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2017-05-05 17:31:29]

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

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

Dry run: CQ is trying da patch.

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

[Ryan Hamilton, 2017-05-05 17:32:48]

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.h:78:
std::unique_ptr<QuicChromiumClientSession::Handle> session_;
On 2017/05/05 14:10:01, xunjieli wrote:
> I guess this can be "const std::unique_ptr<>" ?

Done. (I don't think I've ever seen a const std::unique_ptr<> as a member
before)

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

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:299: new
StreamRequest(session_->CreateHandle(), requires_confirmation));
On 2017/05/05 14:10:01, xunjieli wrote:
> Have you considered the alternative approach below?
> 
> Make StreamRequest as an inner class of the session Handle, so it can access
> Handles members. That way we can avoid creating another Handle here and the
> forward declaration of StreamRequest in the header file.

That's possible, but the StreamRequest is currently passed to the Session so it
would require more gymnastics to make it work out. Though, I'm not sure that the
creation of an additional handle is particularly problematic. I'm happy to take
a look at this in a followup if you think it's worthwhile.

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

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.h:365: // Return a Handle to this
session.
On 2017/05/05 14:10:01, xunjieli wrote:
> minor nit: s/Return/Returns

Done.

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
File net/quic/chromium/quic_http_stream.h (right):

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
net/quic/chromium/quic_http_stream.h:145: const
QuicChromiumClientSession::Handle* quic_session() const {
On 2017/05/05 14:10:01, xunjieli wrote:
> We have two versions of accessors? Can we get rid of one?
> 
> C++ style guide discourages function overloads
> https://google.github.io/styleguide/cppguide.html#Function_Overloading
> 
> "If you want to overload a function, consider qualifying the name with some
> information about the arguments, e.g., AppendString(), AppendInt() rather than
> just Append(). "

Can't get rid of either because we need the const version when calling from
const methods, and we need the non-const one when we want to mutate the session.
 I don't think this usage violates the style guides guidance:

Use overloaded functions (including constructors) only if a reader looking at a
call site can get a good idea of what is happening without having to first
figure out exactly which overload is being called.

Since both of these accessors do exactly the same thing (return a pointer to the
underlying session) it seems pretty clear to  caller what is happening.

(As an asside that the example of AppendString()/AppendInt() is based on the
arguments being different. In this case, it's only the const-ness which is
different)

[xunjieli, 2017-05-05 17:52:13]

lgtm mod the request for a follow-up.

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.h:78:
std::unique_ptr<QuicChromiumClientSession::Handle> session_;
On 2017/05/05 17:32:47, Ryan Hamilton wrote:
> On 2017/05/05 14:10:01, xunjieli wrote:
> > I guess this can be "const std::unique_ptr<>" ?
> 
> Done. (I don't think I've ever seen a const std::unique_ptr<> as a member
> before)

https://cs.chromium.org/search/?q=const+std::unique_ptr+file:%5Esrc/net/+pack...
10 pages of result in net/ :)

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

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:299: new
StreamRequest(session_->CreateHandle(), requires_confirmation));
On 2017/05/05 17:32:47, Ryan Hamilton wrote:
> On 2017/05/05 14:10:01, xunjieli wrote:
> > Have you considered the alternative approach below?
> > 
> > Make StreamRequest as an inner class of the session Handle, so it can access
> > Handles members. That way we can avoid creating another Handle here and the
> > forward declaration of StreamRequest in the header file.
> 
> That's possible, but the StreamRequest is currently passed to the Session so
it
> would require more gymnastics to make it work out. Though, I'm not sure that
the
> creation of an additional handle is particularly problematic. I'm happy to
take
> a look at this in a followup if you think it's worthwhile. 
> 

I feel strongly about this. I think the Handle object should not be all over the
place. It should have a 1-1 relationship with each BidiStream/HttpStream.
Otherwise, this Handle concept would be very different from the concept of, say,
ClientSocketHandle. Since we are aiming for a unifying interface for QUIC/HTTP,
we should try making the similarly named classes comparable. I am fine with a
follow-up CL.

[Ryan Hamilton, 2017-05-05 18:27:38]

Thanks! Landing. (And working on the followup)

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.h:78:
std::unique_ptr<QuicChromiumClientSession::Handle> session_;
On 2017/05/05 17:52:13, xunjieli wrote:
> On 2017/05/05 17:32:47, Ryan Hamilton wrote:
> > On 2017/05/05 14:10:01, xunjieli wrote:
> > > I guess this can be "const std::unique_ptr<>" ?
> > 
> > Done. (I don't think I've ever seen a const std::unique_ptr<> as a member
> > before)
> 
>
https://cs.chromium.org/search/?q=const+std::unique_ptr+file:%5Esrc/net/+pack...
> 10 pages of result in net/ :)

Learn something new every day! :)

Though I think you need "s in your search, because that search matches lines
line:

static const SpdyStreamId kInvalidStream;

So with quotes we get:

https://cs.chromium.org/search/?q=%22const+std::unique_ptr%22+file:%5Esrc/net...

Which is 32 results. But still, more than I knew about! :)

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

https://codereview.chromium.org/2334943002/diff/180001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_session.cc:299: new
StreamRequest(session_->CreateHandle(), requires_confirmation));
On 2017/05/05 17:52:13, xunjieli wrote:
> On 2017/05/05 17:32:47, Ryan Hamilton wrote:
> > On 2017/05/05 14:10:01, xunjieli wrote:
> > > Have you considered the alternative approach below?
> > > 
> > > Make StreamRequest as an inner class of the session Handle, so it can
access
> > > Handles members. That way we can avoid creating another Handle here and
the
> > > forward declaration of StreamRequest in the header file.
> > 
> > That's possible, but the StreamRequest is currently passed to the Session so
> it
> > would require more gymnastics to make it work out. Though, I'm not sure that
> the
> > creation of an additional handle is particularly problematic. I'm happy to
> take
> > a look at this in a followup if you think it's worthwhile. 
> > 
> 
> I feel strongly about this. 

Happy to do this in a follow up. I'll work on it next.

> I think the Handle object should not be all over the
> place. It should have a 1-1 relationship with each BidiStream/HttpStream.
> Otherwise, this Handle concept would be very different from the concept of,
say,
> ClientSocketHandle. 

It's not at all clear to me that this is the the right mental model for a
SessionHandle. Since a ClientSocketHandle is not multiplexed, it's necessarily
1:1 with the object that is currently using it. That is not the case for
sessions. We could totally have a "SessionWatcher" class which is collecting
metrics from a session but which is not a stream, for example. The
QuicConnectionLogger is an example of this.

The Session handle is *not* a handle to the underlying stream. I totally agree
that a handle to the *Stream* needs to be 1:1 with the BidiStream/HttpStream.
But there is no reason to make this guarantee for the SessionHandle.

Something we could try to do down the road (after we have a StreamHandle) is to
hide the SessionHandle from the BidiStream/HttpStream by stuffing it into the
StreamHandle and allowing the StreamHandle to be in a state where it is not yet
bound to an underlying stream and have methods (which delegate to the
SessionHandle) to request an underlying stream. But even doing this does not
require any sort of 1:1 mapping between SessionHandles and BidiStreams.

> Since we are aiming for a unifying interface for QUIC/HTTP,
> we should try making the similarly named classes comparable. I am fine with a
> follow-up CL. 

Happy to do this in a follow up just to hide the class from the callers, even if
I'm not sold on the 1:1 ness :)

[commit-bot: I haz the power, 2017-05-05 18:50:13]

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

[commit-bot: I haz the power, 2017-05-05 18:50:15]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2017-05-05 19:22:52]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2017-05-05 19:24:06]

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-05 19:32:02]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494012172687570,
"parent_rev": "8f6f9c1597928d3df4cd6a44942c323ef8e46fee", "commit_rev":
"f0b18c8a8efc9b2dd335a0b29bd243b0711919a6"}

[commit-bot: I haz the power, 2017-05-05 19:32:11]

Description was changed from

==========
The Handle is owned by the holder of the Handle and can outlive the session. In
the event that it does outlive the session, it will record state from the
session in member variables so that the holder of a session need not be aware of
the lifetime of the underlying session.

As a consequence, this change eliminates the need for the
QuicChromiumClientSession::Observer interface, and hence eliminates the upcalls
from the session to the streams. There are still upcalls from the QuicStream to
the QuicHttpStream (and friends) but those can be removed in subsequent CLs by
following the same pattern.

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

to

==========
The Handle is owned by the holder of the Handle and can outlive the session. In
the event that it does outlive the session, it will record state from the
session in member variables so that the holder of a session need not be aware of
the lifetime of the underlying session.

As a consequence, this change eliminates the need for the
QuicChromiumClientSession::Observer interface, and hence eliminates the upcalls
from the session to the streams. There are still upcalls from the QuicStream to
the QuicHttpStream (and friends) but those can be removed in subsequent CLs by
following the same pattern.

BUG=716563

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

[commit-bot: I haz the power, 2017-05-05 19:32:12]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/f0b18c8a8efc9b2dd335a0b29bd2...