Implement timeouts for IQ requests.

remoting/protocol/jingle_session.cc

Index: remoting/protocol/jingle_session.cc
diff --git a/remoting/protocol/jingle_session.cc b/remoting/protocol/jingle_session.cc
index 61d4bc975c72a17ea60b9f01c837808c2d0cf608..32cf7f3b10078aa5538c40b1166bfc47d99013e2 100644
--- a/remoting/protocol/jingle_session.cc
+++ b/remoting/protocol/jingle_session.cc
@@ -32,6 +32,11 @@ namespace {
 // process.
 const int kTransportInfoSendDelayMs = 2;
 
+// How long we should wait for a response from the other end. This
+// value is used for all requests include |session-initiate| and
+// |transport-info|.
+const int kMessageResponseTimeoutSeconds = 10;
+
 Session::Error AuthRejectionReasonToError(
     Authenticator::RejectionReason reason) {
   switch (reason) {
@@ -102,10 +107,7 @@ void JingleSession::StartConnection(
   message.description.reset(
       new ContentDescription(candidate_config_->Clone(),
                              authenticator_->GetNextMessage()));
-  initiate_request_ = session_manager_->iq_sender()->SendIq(
-      message.ToXml(),
-      base::Bind(&JingleSession::OnSessionInitiateResponse,
-                 base::Unretained(this)));
+  SendMessage(message);
 
   SetState(CONNECTING);
 }
@@ -158,10 +160,7 @@ void JingleSession::AcceptIncomingConnection(
   message.description.reset(
       new ContentDescription(CandidateSessionConfig::CreateFrom(config_),
                              auth_message.Pass()));
-  initiate_request_ = session_manager_->iq_sender()->SendIq(
-      message.ToXml(),
-      base::Bind(&JingleSession::OnSessionInitiateResponse,
-                 base::Unretained(this)));
+  SendMessage(message);
 
   // Update state.
   SetState(CONNECTED);
@@ -175,20 +174,6 @@ void JingleSession::AcceptIncomingConnection(
   return;
 }
 
-void JingleSession::OnSessionInitiateResponse(
-    const buzz::XmlElement* response) {
-  const std::string& type = response->Attr(buzz::QName("", "type"));
-  if (type != "result") {
-    LOG(ERROR) << "Received error in response to session-initiate message: \""
-               << response->Str()
-               << "\". Terminating the session.";
-
-    // TODO(sergeyu): There may be different reasons for error
-    // here. Parse the response stanza to find failure reason.
-    CloseInternal(PEER_IS_OFFLINE);
-  }
-}
-
 void JingleSession::CreateStreamChannel(
       const std::string& name,
       const StreamChannelCallback& callback) {
@@ -282,6 +267,18 @@ void JingleSession::OnTransportDeleted(Transport* transport) {
   channels_.erase(it);
 }
 
+void JingleSession::SendMessage(const JingleMessage& message) {
+  scoped_ptr<IqRequest> request = session_manager_->iq_sender()->SendIq(
+      message.ToXml(),
+      base::Bind(&JingleSession::OnIqMessageResponse,
+                 base::Unretained(this), message.action));
+  if (request.get()) {
+    request->SetTimeout(
+        base::TimeDelta::FromSeconds(kMessageResponseTimeoutSeconds));
+    pending_requests_.push_back(request.release());
+  }

[simonmorris, 2012/02/24 00:11:31]

Log an error if request.get() is NULL?

[Sergey Ulanov, 2012/02/24 21:56:31]

Done.

+}
+
 void JingleSession::OnIncomingMessage(const JingleMessage& message,
                                       const ReplyCallback& reply_callback) {
   DCHECK(CalledOnValidThread());
@@ -451,11 +448,7 @@ void JingleSession::ProcessAuthenticationStep() {
     JingleMessage message(peer_jid_, JingleMessage::SESSION_INFO, session_id_);
     message.info = authenticator_->GetNextMessage();
     DCHECK(message.info.get());
-
-    session_info_request_ = session_manager_->iq_sender()->SendIq(
-        message.ToXml(), base::Bind(
-            &JingleSession::OnSessionInfoResponse,
-            base::Unretained(this)));
+    SendMessage(message);
   }
   DCHECK_NE(authenticator_->state(), Authenticator::MESSAGE_READY);
 
@@ -467,42 +460,72 @@ void JingleSession::ProcessAuthenticationStep() {
   }
 }
 
-void JingleSession::OnSessionInfoResponse(const buzz::XmlElement* response) {
-  const std::string& type = response->Attr(buzz::QName("", "type"));
-  if (type != "result") {
-    LOG(ERROR) << "Received error in response to session-info message: \""
-               << response->Str()
-               << "\". Terminating the session.";
-    CloseInternal(INCOMPATIBLE_PROTOCOL);
+void JingleSession::OnIqMessageResponse(
+    JingleMessage::ActionType request_type,
+    IqRequest* request,
+    const buzz::XmlElement* response) {
+  Error error = OK;
+
+  std::string type_str = JingleMessage::GetActionName(request_type);
+
+  if (!response) {
+    LOG(ERROR) << type_str << " request timed out.";
+    // Most likely the session-initiate timeout indicates a problem
+    // with the signaling.
+    error = UNKNOWN_ERROR;
+  } else {
+    const std::string& type = response->Attr(buzz::QName("", "type"));
+    if (type != "result") {
+      LOG(ERROR) << "Received error in response to " << type_str
+                 << " message: \"" << response->Str()
+                 << "\". Terminating the session.";
+
+      switch (request_type) {
+        case JingleMessage::SESSION_INFO:
+          // session-info is used for the new authentication protocol,
+          // and wasn't previously supported.
+          error = INCOMPATIBLE_PROTOCOL;
+
+        default:
+          // TODO(sergeyu): There may be different reasons for error
+          // here. Parse the response stanza to find failure reason.
+          error = PEER_IS_OFFLINE;
+      }
+    }
+  }
+
+  CleanupPendingRequests(request);
+
+  if (error != OK) {
+    CloseInternal(error);
   }
 }
 
-void JingleSession::OnTransportInfoResponse(const buzz::XmlElement* response) {
-  const std::string& type = response->Attr(buzz::QName("", "type"));
-  if (type != "result") {
-    LOG(ERROR) << "Received error in response to session-initiate message: \""
-               << response->Str()
-               << "\". Terminating the session.";
+void JingleSession::CleanupPendingRequests(IqRequest* request) {
+  DCHECK(!pending_requests_.empty());
+  DCHECK(request);
 
-    if (state_ == CONNECTING) {
-      CloseInternal(PEER_IS_OFFLINE);
-    } else {
-      // Host has disconnected without sending session-terminate message.
-      CloseInternal(OK);
-    }
+  // This method is called whenever a response to |request| is
+  // received. Here we delete that request and all requests that were
+  // sent before it.

[simonmorris, 2012/02/24 00:11:31]

Why delete all requests sent before the given request?

[Sergey Ulanov, 2012/02/24 21:56:31]

The logic here is that if we send messages A, B and C and then suddenly receive
response to C then it means that responses to A and B were somehow lost. I think
that may happen only when the client switches from one network to another.
Ignoring timeouts for A and B seems to be the right thing to do in this case.
Added more comments about it.

[simonmorris, 2012/02/24 22:32:51]

The simple thing to do is just let A and B timeout as usual.  Under what
circumstances does that have a downside?

It's harder to reason about a system where there are three outcomes of sending a
message (response, timeout, ignore) instead of two (response, timeout). A future
developer could easily neglect the third case, especially if it happens
infrequently.

[Sergey Ulanov, 2012/02/25 00:11:52]

That would mean that the session will terminate even when we know that the peer
is still online. Not the best user experience.
That's only for the code that use JingleSession::SendMessage() and it uses the
same handler for all responses, so I don't think it makes big difference here.
In any case I added comments next to SendMessage() to explain its behavior.

[simonmorris, 2012/02/25 00:25:21]

OK - if it's clear that a session can be considered alive with a response to C,
but no responses to A and B, then this makes sense.
OK.

+  while (!pending_requests_.empty() && pending_requests_.front() != request) {
+    delete pending_requests_.front();
+    pending_requests_.pop_front();
   }
+
+  // Delete the |request| itself.
+  DCHECK_EQ(request, pending_requests_.front());
+  delete request;
+  if (!pending_requests_.empty())
+    pending_requests_.pop_front();
 }
 
 void JingleSession::SendTransportInfo() {
   JingleMessage message(peer_jid_, JingleMessage::TRANSPORT_INFO, session_id_);
   message.candidates.swap(pending_candidates_);
-  transport_info_request_ = session_manager_->iq_sender()->SendIq(
-      message.ToXml(), base::Bind(
-          &JingleSession::OnTransportInfoResponse,
-          base::Unretained(this)));
+  SendMessage(message);
 }
 
-
 void JingleSession::CloseInternal(Error error) {
   DCHECK(CalledOnValidThread());
 
@@ -527,8 +550,7 @@ void JingleSession::CloseInternal(Error error) {
     JingleMessage message(peer_jid_, JingleMessage::SESSION_TERMINATE,
                           session_id_);
     message.reason = reason;
-    session_manager_->iq_sender()->SendIq(
-        message.ToXml(), IqSender::ReplyCallback());
+    SendMessage(message);
   }
 
   error_ = error;