Implement timeouts for IQ requests.

remoting/protocol/jingle_session.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/protocol/jingle_session.h"
 
 #include "base/bind.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
@@ skipping 14 matching lines @@
 namespace protocol {
 
 namespace {
 // Delay after candidate creation before sending transport-info
 // message. This is neccessary to be able to pack multiple candidates
 // into one transport-info messages. The value needs to be greater
 // than zero because ports are opened asynchronously in the browser
 // 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) {
     case Authenticator::INVALID_CREDENTIALS:
       return Session::AUTHENTICATION_FAILED;
     case Authenticator::PROTOCOL_ERROR:
       return Session::INCOMPATIBLE_PROTOCOL;
   }
   NOTREACHED();
   return Session::UNKNOWN_ERROR;
@@ skipping 50 matching lines @@
   // enough entropy. In the worst case connection will fail when two
   // clients generate the same session ID concurrently.
   session_id_ = base::Int64ToString(base::RandGenerator(kint64max));
 
   // Send session-initiate message.
   JingleMessage message(peer_jid_, JingleMessage::SESSION_INITIATE,
                         session_id_);
   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);
 }
 
 void JingleSession::InitializeIncomingConnection(
     const JingleMessage& initiate_message,
     scoped_ptr<Authenticator> authenticator) {
   DCHECK(CalledOnValidThread());
   DCHECK(initiate_message.description.get());
   DCHECK(authenticator.get());
@@ skipping 32 matching lines @@
   JingleMessage message(peer_jid_, JingleMessage::SESSION_ACCEPT,
                         session_id_);
 
   scoped_ptr<buzz::XmlElement> auth_message;
   if (authenticator_->state() == Authenticator::MESSAGE_READY)
     auth_message = authenticator_->GetNextMessage();
 
   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);
 
   if (authenticator_->state() == Authenticator::ACCEPTED) {
     SetState(AUTHENTICATED);
   } else {
     DCHECK_EQ(authenticator_->state(), Authenticator::WAITING_MESSAGE);
   }
 
   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) {
   DCHECK(!channels_[name]);
 
   scoped_ptr<ChannelAuthenticator> channel_authenticator =
       authenticator_->CreateChannelAuthenticator();
   scoped_ptr<StreamTransport> channel =
       session_manager_->transport_factory_->CreateStreamTransport();
   channel->Initialize(name, session_manager_->transport_config_,
@@ skipping 73 matching lines @@
                                route.local_address);
   }
 }
 
 void JingleSession::OnTransportDeleted(Transport* transport) {
   ChannelsMap::iterator it = channels_.find(transport->name());
   DCHECK_EQ(it->second, 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());
 
   if (message.from != peer_jid_) {
     // Ignore messages received from a different Jid.
     reply_callback.Run(JingleMessageReply::INVALID_SID);
     return;
   }
 
@@ skipping 149 matching lines @@
   return true;
 }
 
 void JingleSession::ProcessAuthenticationStep() {
   DCHECK_EQ(state_, CONNECTED);
 
   if (authenticator_->state() == Authenticator::MESSAGE_READY) {
     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);
 
   if (authenticator_->state() == Authenticator::ACCEPTED) {
     SetState(AUTHENTICATED);
   } else if (authenticator_->state() == Authenticator::REJECTED) {
     CloseInternal(AuthRejectionReasonToError(
         authenticator_->rejection_reason()));
   }
 }
 
-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());
 
   if (state_ == CONNECTING || state_ == CONNECTED || state_ == AUTHENTICATED) {
     // Send session-terminate message with the appropriate error code.
     JingleMessage::Reason reason;
     switch (error) {
       case OK:
         reason = JingleMessage::SUCCESS;
         break;
       case SESSION_REJECTED:
       case AUTHENTICATION_FAILED:
         reason = JingleMessage::DECLINE;
         break;
       case INCOMPATIBLE_PROTOCOL:
         reason = JingleMessage::INCOMPATIBLE_PARAMETERS;
         break;
       default:
         reason = JingleMessage::GENERAL_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;
 
   if (state_ != FAILED && state_ != CLOSED) {
     if (error != OK) {
       SetState(FAILED);
     } else {
       SetState(CLOSED);
     }
   }
 }
 
 void JingleSession::SetState(State new_state) {
   DCHECK(CalledOnValidThread());
 
   if (new_state != state_) {
     DCHECK_NE(state_, CLOSED);
     DCHECK_NE(state_, FAILED);
 
     state_ = new_state;
     if (!state_change_callback_.is_null())
       state_change_callback_.Run(new_state);
   }
 }
 
 }  // namespace protocol
 }  // namespace remoting