[remoting host] Add stream ready timeout for stream requests

Stream requests unlike unary requests don't set the request timeout,
since that would simply destroy the stream if it hasn't been closed
within the timeout.

This CL adds a OneShotTimer to ProtobufHttpStreamRequest to run the
StreamClosedCallback with DEADLINE_EXCEEDED if the stream does not
receive its first data within the timeout.

Bug: 1123199
Change-Id: Iaeb9c5c4f896d2a24378bb83c57d2d39ab49312c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2386280
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803615}

remoting/base/protobuf_http_client_unittest.cc

@@ -560,4 +560,33 @@ TEST_F(ProtobufHttpClientTest, SendStreamStatusAndHttpStatus_StreamStatusWins) {
   ASSERT_FALSE(client_.HasPendingRequests());
 }
 
+TEST_F(ProtobufHttpClientTest, StreamReadyTimeout) {
+  base::MockOnceClosure not_called_stream_ready_callback;
+  MockEchoMessageCallback not_called_message_callback;
+  MockStreamClosedCallback stream_closed_callback;
+
+  {
+    InSequence s;
+
+    ExpectCallWithToken(/* success= */ true);
+    EXPECT_CALL(stream_closed_callback,
+                Run(HasErrorCode(ProtobufHttpStatus::Code::DEADLINE_EXCEEDED)));
+  }
+
+  auto request = CreateDefaultTestStreamRequest();
+  request->SetStreamReadyCallback(not_called_stream_ready_callback.Get());
+  request->SetMessageCallback(not_called_message_callback.Get());
+  request->SetStreamClosedCallback(stream_closed_callback.Get());
+  client_.ExecuteRequest(std::move(request));
+
+  ASSERT_TRUE(client_.HasPendingRequests());
+  ASSERT_TRUE(test_url_loader_factory_.IsPending(kTestFullUrl));
+  ASSERT_EQ(1, test_url_loader_factory_.NumPending());
+
+  task_environment_.FastForwardBy(
+      ProtobufHttpStreamRequest::kStreamReadyTimeoutDuration +
+      base::TimeDelta::FromSeconds(1));
+  ASSERT_FALSE(client_.HasPendingRequests());
+}
+
 }  // namespace remoting

remoting/base/protobuf_http_stream_request.cc

@@ -15,6 +15,10 @@
 
 namespace remoting {
 
+// static
+constexpr base::TimeDelta
+    ProtobufHttpStreamRequest::kStreamReadyTimeoutDuration;
+
 ProtobufHttpStreamRequest::ProtobufHttpStreamRequest(
     std::unique_ptr<ProtobufHttpRequestConfig> config)
     : ProtobufHttpRequestBase(std::move(config)) {}
@@ -63,6 +67,11 @@ void ProtobufHttpStreamRequest::StartRequestInternal(
   DCHECK(stream_ready_callback_);
   DCHECK(stream_closed_callback_);
   DCHECK(message_callback_);
+  DCHECK(!stream_ready_timeout_timer_.IsRunning());
+
+  stream_ready_timeout_timer_.Start(
+      FROM_HERE, kStreamReadyTimeoutDuration, this,
+      &ProtobufHttpStreamRequest::OnStreamReadyTimeout);
 
   // Safe to use unretained, as callbacks won't be called after |stream_parser_|
   // is deleted.
@@ -80,6 +89,10 @@ base::TimeDelta ProtobufHttpStreamRequest::GetRequestTimeoutDuration() const {
 
 void ProtobufHttpStreamRequest::OnDataReceived(base::StringPiece string_piece,
                                                base::OnceClosure resume) {
+  if (stream_ready_timeout_timer_.IsRunning()) {
+    stream_ready_timeout_timer_.Stop();
+  }
+
   if (stream_ready_callback_) {
     std::move(stream_ready_callback_).Run();
   }
@@ -98,4 +111,9 @@ void ProtobufHttpStreamRequest::OnRetry(base::OnceClosure start_retry) {
   NOTIMPLEMENTED();
 }
 
+void ProtobufHttpStreamRequest::OnStreamReadyTimeout() {
+  OnStreamClosed(ProtobufHttpStatus(ProtobufHttpStatus::Code::DEADLINE_EXCEEDED,
+                                    "Stream connection failed: timeout"));
+}
+
 }  // namespace remoting

remoting/base/protobuf_http_stream_request.h

@@ -7,6 +7,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/timer/timer.h"
 #include "remoting/base/protobuf_http_request_base.h"
 #include "services/network/public/cpp/simple_url_loader_stream_consumer.h"
 
@@ -33,6 +34,9 @@ class ProtobufHttpStreamRequest final
   using StreamClosedCallback =
       base::OnceCallback<void(const ProtobufHttpStatus& status)>;
 
+  static constexpr base::TimeDelta kStreamReadyTimeoutDuration =
+      base::TimeDelta::FromSeconds(30);
+
   explicit ProtobufHttpStreamRequest(
       std::unique_ptr<ProtobufHttpRequestConfig> config);
   ~ProtobufHttpStreamRequest() override;
@@ -57,9 +61,6 @@ class ProtobufHttpStreamRequest final
         callback);
   }
 
-  // TODO(yuweih): Consider adding an option to set timeout for
-  // |stream_ready_callback_|.
-
  private:
   friend class ProtobufHttpClient;
 
@@ -79,11 +80,15 @@ class ProtobufHttpStreamRequest final
   void OnComplete(bool success) override;
   void OnRetry(base::OnceClosure start_retry) override;
 
+  void OnStreamReadyTimeout();
+
   // Used to create new response message instances.
   const google::protobuf::MessageLite* default_message_;
 
   std::unique_ptr<ProtobufHttpStreamParser> stream_parser_;
 
+  base::OneShotTimer stream_ready_timeout_timer_;
+
   base::OnceClosure stream_ready_callback_;
   StreamClosedCallback stream_closed_callback_;
   base::RepeatingCallback<void(std::unique_ptr<google::protobuf::MessageLite>)>