Add a new QuicChromiumClientSession::Handle class

net/quic/chromium/quic_chromium_client_session.cc

Index: net/quic/chromium/quic_chromium_client_session.cc
diff --git a/net/quic/chromium/quic_chromium_client_session.cc b/net/quic/chromium/quic_chromium_client_session.cc
index cd123b19de48d537ea0477a194e63ba669259869..4255d8aae3c4e677d2393eaa49ed723f5a053698 100644
--- a/net/quic/chromium/quic_chromium_client_session.cc
+++ b/net/quic/chromium/quic_chromium_client_session.cc
@@ -186,26 +186,185 @@ class QuicServerPushHelper : public ServerPushDelegate::ServerPushHelper {
 
 }  // namespace
 
+QuicChromiumClientSession::Handle::Handle(
+    const base::WeakPtr<QuicChromiumClientSession>& session)
+    : MultiplexedSessionHandle(session),
+      session_(session),
+      net_log_(session_->net_log()),
+      was_handshake_confirmed_(session->IsCryptoHandshakeConfirmed()),
+      error_(OK),
+      port_migration_detected_(false),
+      server_id_(session_->server_id()),
+      quic_version_(session->connection()->version()) {
+  DCHECK(session_);
+  session_->AddHandle(this);
+}
+
+QuicChromiumClientSession::Handle::~Handle() {
+  if (session_)
+    session_->RemoveHandle(this);
+}
+
+void QuicChromiumClientSession::Handle::OnCryptoHandshakeConfirmed() {
+  was_handshake_confirmed_ = true;
+}
+
+void QuicChromiumClientSession::Handle::OnSessionClosed(
+    QuicVersion quic_version,
+    int error,
+    bool port_migration_detected,
+    LoadTimingInfo::ConnectTiming connect_timing) {
+  session_ = nullptr;
+  port_migration_detected_ = port_migration_detected;
+  error_ = error;
+  quic_version_ = quic_version;
+  connect_timing_ = connect_timing;
+}
+
+bool QuicChromiumClientSession::Handle::IsConnected() const {
+  return session_ != nullptr;
+}
+
+bool QuicChromiumClientSession::Handle::IsCryptoHandshakeConfirmed() const {
+  return was_handshake_confirmed_;
+}
+
+const LoadTimingInfo::ConnectTiming&
+QuicChromiumClientSession::Handle::GetConnectTiming() {
+  if (!session_)
+    return connect_timing_;
+
+  return session_->GetConnectTiming();
+}
+
+Error QuicChromiumClientSession::Handle::GetTokenBindingSignature(
+    crypto::ECPrivateKey* key,
+    TokenBindingType tb_type,
+    std::vector<uint8_t>* out) {
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  return session_->GetTokenBindingSignature(key, tb_type, out);
+}
+
+void QuicChromiumClientSession::Handle::PopulateNetErrorDetails(
+    NetErrorDetails* details) const {
+  if (session_) {
+    session_->PopulateNetErrorDetails(details);
+  } else {
+    details->quic_port_migration_detected = port_migration_detected_;
+  }
+}
+
+QuicVersion QuicChromiumClientSession::Handle::GetQuicVersion() const {
+  if (!session_)
+    return quic_version_;
+
+  return session_->connection()->version();
+}
+
+void QuicChromiumClientSession::Handle::ResetPromised(
+    QuicStreamId id,
+    QuicRstStreamErrorCode error_code) {
+  if (session_)
+    session_->ResetPromised(id, error_code);
+}
+
+std::unique_ptr<QuicConnection::ScopedPacketBundler>
+QuicChromiumClientSession::Handle::CreatePacketBundler(
+    QuicConnection::AckBundling bundling_mode) {
+  if (!session_)
+    return nullptr;
+
+  return base::MakeUnique<QuicConnection::ScopedPacketBundler>(
+      session_->connection(), bundling_mode);
+}
+
+bool QuicChromiumClientSession::Handle::SharesSameSession(
+    const Handle& other) const {
+  return session_.get() == other.session_.get();
+}
+
+int QuicChromiumClientSession::Handle::RequestStream(
+    bool requires_confirmation,
+    const CompletionCallback& callback) {
+  DCHECK(!stream_request_);
+
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  // base::MakeUnique does not work because the StreamRequest constructor
+  // is private.
+  stream_request_ = std::unique_ptr<StreamRequest>(
+      new StreamRequest(session_->CreateHandle(), requires_confirmation));

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

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.

[Ryan Hamilton, 2017/05/05 17:32:47]

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. 

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

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]

Happy to do this in a follow up. I'll work on it next.
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.
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 :)

+  return stream_request_->StartRequest(callback);
+}
+
+QuicChromiumClientStream* QuicChromiumClientSession::Handle::ReleaseStream() {
+  DCHECK(stream_request_);
+
+  auto* stream = stream_request_->ReleaseStream();
+  stream_request_.reset();
+  return stream;
+}
+
+int QuicChromiumClientSession::Handle::WaitForHandshakeConfirmation(
+    const CompletionCallback& callback) {
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  return session_->WaitForHandshakeConfirmation(callback);
+}
+
+void QuicChromiumClientSession::Handle::CancelRequest(StreamRequest* request) {
+  if (session_)
+    session_->CancelRequest(request);
+}
+
+int QuicChromiumClientSession::Handle::TryCreateStream(StreamRequest* request) {
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  return session_->TryCreateStream(request);
+}
+
+QuicClientPushPromiseIndex*
+QuicChromiumClientSession::Handle::GetPushPromiseIndex() {
+  if (!session_)
+    return push_promise_index_;
+
+  return session_->push_promise_index();
+}
+
+int QuicChromiumClientSession::Handle::GetPeerAddress(
+    IPEndPoint* address) const {
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  *address = session_->peer_address().impl().socket_address();
+  return OK;
+}
+
 QuicChromiumClientSession::StreamRequest::StreamRequest(
-    const base::WeakPtr<QuicChromiumClientSession>& session,
+    std::unique_ptr<QuicChromiumClientSession::Handle> session,
     bool requires_confirmation)
-    : session_(session),
+    : session_(std::move(session)),
       requires_confirmation_(requires_confirmation),
       stream_(nullptr),
-      next_state_(STATE_NONE),
       weak_factory_(this) {}
 
 QuicChromiumClientSession::StreamRequest::~StreamRequest() {
   if (stream_)
     stream_->Reset(QUIC_STREAM_CANCELLED);
 
-  if (session_)
+  if (session_->IsConnected())
     session_->CancelRequest(this);
 }
 
 int QuicChromiumClientSession::StreamRequest::StartRequest(
     const CompletionCallback& callback) {
-  DCHECK(session_);
+  if (!session_->IsConnected())
+    return ERR_CONNECTION_CLOSED;
 
   next_state_ = STATE_WAIT_FOR_CONFIRMATION;
   int rv = DoLoop(OK);
@@ -226,7 +385,7 @@ QuicChromiumClientSession::StreamRequest::ReleaseStream() {
 void QuicChromiumClientSession::StreamRequest::OnRequestCompleteSuccess(
     QuicChromiumClientStream* stream) {
   DCHECK_EQ(STATE_REQUEST_STREAM_COMPLETE, next_state_);
-  session_.reset();
+
   stream_ = stream;
   // This method is called even when the request completes synchronously.
   if (callback_)
@@ -236,7 +395,6 @@ void QuicChromiumClientSession::StreamRequest::OnRequestCompleteSuccess(
 void QuicChromiumClientSession::StreamRequest::OnRequestCompleteFailure(
     int rv) {
   DCHECK_EQ(STATE_REQUEST_STREAM_COMPLETE, next_state_);
-  session_.reset();
   // This method is called even when the request completes synchronously.
   if (callback_)
     DoCallback(rv);
@@ -316,7 +474,6 @@ int QuicChromiumClientSession::StreamRequest::DoRequestStream() {
 
 int QuicChromiumClientSession::StreamRequest::DoRequestStreamComplete(int rv) {
   DCHECK(rv == OK || !stream_);
-  session_.reset();
 
   return rv;
 }
@@ -399,18 +556,18 @@ QuicChromiumClientSession::~QuicChromiumClientSession() {
   DCHECK(waiting_for_confirmation_callbacks_.empty());
   if (!dynamic_streams().empty())
     RecordUnexpectedOpenStreams(DESTRUCTOR);
-  if (!observers_.empty())
+  if (!handles_.empty())
     RecordUnexpectedObservers(DESTRUCTOR);
   if (!going_away_)
     RecordUnexpectedNotGoingAway(DESTRUCTOR);
 
-  while (!dynamic_streams().empty() || !observers_.empty() ||
+  while (!dynamic_streams().empty() || !handles_.empty() ||
          !stream_requests_.empty()) {
     // The session must be closed before it is destroyed.
     DCHECK(dynamic_streams().empty());
     CloseAllStreams(ERR_UNEXPECTED);
-    DCHECK(observers_.empty());
-    CloseAllObservers(ERR_UNEXPECTED);
+    DCHECK(handles_.empty());
+    CloseAllHandles(ERR_UNEXPECTED);
     CancelAllRequests(ERR_UNEXPECTED);
 
     connection()->set_debug_visitor(nullptr);
@@ -528,28 +685,23 @@ void QuicChromiumClientSession::OnStreamFrame(const QuicStreamFrame& frame) {
   return QuicSpdySession::OnStreamFrame(frame);
 }
 
-void QuicChromiumClientSession::AddObserver(Observer* observer) {
+void QuicChromiumClientSession::AddHandle(Handle* handle) {
   if (going_away_) {
     RecordUnexpectedObservers(ADD_OBSERVER);
-    observer->OnSessionClosed(ERR_UNEXPECTED, port_migration_detected_);
+    handle->OnSessionClosed(connection()->version(), ERR_UNEXPECTED,
+                            port_migration_detected_,
+
+                            GetConnectTiming());
     return;
   }
 
-  DCHECK(!base::ContainsKey(observers_, observer));
-  observers_.insert(observer);
-}
-
-void QuicChromiumClientSession::RemoveObserver(Observer* observer) {
-  DCHECK(base::ContainsKey(observers_, observer));
-  observers_.erase(observer);
+  DCHECK(!base::ContainsKey(handles_, handle));
+  handles_.insert(handle);
 }
 
-std::unique_ptr<QuicChromiumClientSession::StreamRequest>
-QuicChromiumClientSession::CreateStreamRequest(bool requires_confirmation) {
-  // base::MakeUnique does not work because the StreamRequest constructor
-  // is private.
-  return std::unique_ptr<StreamRequest>(
-      new StreamRequest(weak_factory_.GetWeakPtr(), requires_confirmation));
+void QuicChromiumClientSession::RemoveHandle(Handle* handle) {
+  DCHECK(base::ContainsKey(handles_, handle));
+  handles_.erase(handle);
 }
 
 int QuicChromiumClientSession::WaitForHandshakeConfirmation(
@@ -960,11 +1112,11 @@ void QuicChromiumClientSession::OnCryptoHandshakeEvent(
           base::TimeTicks::Now() - connect_timing_.dns_end);
     }
 
-    ObserverSet::iterator it = observers_.begin();
-    while (it != observers_.end()) {
-      Observer* observer = *it;
+    HandleSet::iterator it = handles_.begin();
+    while (it != handles_.end()) {
+      Handle* handle = *it;
       ++it;
-      observer->OnCryptoHandshakeConfirmed();
+      handle->OnCryptoHandshakeConfirmed();
     }
 
     NotifyRequestsOfConfirmation(OK);
@@ -1108,7 +1260,7 @@ void QuicChromiumClientSession::OnConnectionClosed(
   }
   DCHECK(dynamic_streams().empty());
   CloseAllStreams(ERR_UNEXPECTED);
-  CloseAllObservers(ERR_UNEXPECTED);
+  CloseAllHandles(ERR_UNEXPECTED);
   CancelAllRequests(ERR_CONNECTION_CLOSED);
   NotifyRequestsOfConfirmation(ERR_CONNECTION_CLOSED);
   NotifyFactoryOfSessionClosedLater();
@@ -1118,13 +1270,6 @@ void QuicChromiumClientSession::OnSuccessfulVersionNegotiation(
     const QuicVersion& version) {
   logger_->OnSuccessfulVersionNegotiation(version);
   QuicSpdySession::OnSuccessfulVersionNegotiation(version);
-
-  ObserverSet::iterator it = observers_.begin();
-  while (it != observers_.end()) {
-    Observer* observer = *it;
-    ++it;
-    observer->OnSuccessfulVersionNegotiation(version);
-  }
 }
 
 int QuicChromiumClientSession::HandleWriteError(
@@ -1330,7 +1475,7 @@ void QuicChromiumClientSession::CloseSessionOnError(int net_error,
     base::ResetAndReturn(&callback_).Run(net_error);
   }
   CloseAllStreams(net_error);
-  CloseAllObservers(net_error);
+  CloseAllHandles(net_error);
   net_log_.AddEvent(NetLogEventType::QUIC_SESSION_CLOSE_ON_ERROR,
                     NetLog::IntCallback("net_error", net_error));
 
@@ -1351,11 +1496,12 @@ void QuicChromiumClientSession::CloseAllStreams(int net_error) {
   }
 }
 
-void QuicChromiumClientSession::CloseAllObservers(int net_error) {
-  while (!observers_.empty()) {
-    Observer* observer = *observers_.begin();
-    observers_.erase(observer);
-    observer->OnSessionClosed(net_error, port_migration_detected_);
+void QuicChromiumClientSession::CloseAllHandles(int net_error) {
+  while (!handles_.empty()) {
+    Handle* handle = *handles_.begin();
+    handles_.erase(handle);
+    handle->OnSessionClosed(connection()->version(), net_error,
+                            port_migration_detected_, GetConnectTiming());
   }
 }
 
@@ -1410,9 +1556,10 @@ std::unique_ptr<base::Value> QuicChromiumClientSession::GetInfoAsValue(
   return std::move(dict);
 }
 
-base::WeakPtr<QuicChromiumClientSession>
-QuicChromiumClientSession::GetWeakPtr() {
-  return weak_factory_.GetWeakPtr();
+std::unique_ptr<QuicChromiumClientSession::Handle>
+QuicChromiumClientSession::CreateHandle() {
+  return base::MakeUnique<QuicChromiumClientSession::Handle>(
+      weak_factory_.GetWeakPtr());
 }
 
 void QuicChromiumClientSession::OnReadError(
@@ -1508,7 +1655,7 @@ bool QuicChromiumClientSession::MigrateToSocket(
 }
 
 void QuicChromiumClientSession::PopulateNetErrorDetails(
-    NetErrorDetails* details) {
+    NetErrorDetails* details) const {
   details->quic_port_migration_detected = port_migration_detected_;
 }