JobController 1: Adding a new class HttpStreamFactoryImpl::JobController

net/http/http_stream_factory_impl_job_controller_unittest.cc

Index: net/http/http_stream_factory_impl_job_controller_unittest.cc
diff --git a/net/http/http_stream_factory_impl_job_controller_unittest.cc b/net/http/http_stream_factory_impl_job_controller_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..593fe453939e45d22a0795cd8ae2a5bc3e54d361
--- /dev/null
+++ b/net/http/http_stream_factory_impl_job_controller_unittest.cc
@@ -0,0 +1,267 @@
+// Copyright (c) 2016 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 "net/http/http_stream_factory_impl_job_controller.h"
+
+#include <memory>
+
+#include "net/http/http_basic_stream.h"
+#include "net/http/http_stream_factory_impl_job.h"
+#include "net/http/http_stream_factory_impl_request.h"
+#include "net/http/http_stream_factory_test_util.h"
+#include "net/proxy/proxy_info.h"
+#include "net/proxy/proxy_service.h"
+#include "net/spdy/spdy_test_util_common.h"
+#include "net/ssl/ssl_failure_state.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::_;
+using ::testing::Invoke;
+
+namespace net {
+
+void DeleteHttpStreamPointer(const SSLConfig& used_ssl_config,
+                             const ProxyInfo& used_proxy_info,
+                             HttpStream* stream) {
+  delete stream;
+}
+
+class HttpStreamFactoryImplJobControllerTest
+    : public ::testing::Test,
+      public ::testing::WithParamInterface<NextProto> {
+ public:
+  HttpStreamFactoryImplJobControllerTest()
+      : session_deps_(GetParam(), ProxyService::CreateDirect()) {
+    session_deps_.enable_quic = true;
+    session_ = SpdySessionDependencies::SpdyCreateSession(&session_deps_);
+    factory_ =
+        static_cast<HttpStreamFactoryImpl*>(session_->http_stream_factory());
+    job_controller_ = new HttpStreamFactoryImpl::JobController(
+        factory_, &request_delegate_, session_.get(), &job_factory_);
+    factory_->job_controller_set_.insert(base::WrapUnique(job_controller_));
+  }
+
+  ~HttpStreamFactoryImplJobControllerTest() {}
+
+  void SetAlternativeService(const HttpRequestInfo& request_info,
+                             AlternativeService alternative_service) {
+    HostPortPair host_port_pair = HostPortPair::FromURL(request_info.url);
+    GURL original_url = job_controller_->ApplyHostMappingRules(request_info.url,
+                                                               &host_port_pair);

[Ryan Hamilton, 2016/06/06 17:57:31]

nit: Is this line needed? I'm guessing not, because we don't set up any host
rules later. 

[Zhongyi Shi, 2016/06/06 22:50:55]

Removed. 

+    url::SchemeHostPort server(request_info.url);
+    base::Time expiration = base::Time::Now() + base::TimeDelta::FromDays(1);
+    session_->http_server_properties()->SetAlternativeService(
+        server, alternative_service, expiration);
+  }
+
+  // Set stream to job for test purpose so that job will have stream ready for
+  // Request it's serving.
+  void SetStreamToJob(HttpStream* http_stream,
+                      HttpStreamFactoryImpl::Job* job) {
+    job->SetStream(http_stream);

[Ryan Hamilton, 2016/06/06 17:57:31]

nit: Any reason not to simply inline the call to SetStream() where this method
is called? Doesn't seem like it's adding much value.

[Zhongyi Shi, 2016/06/06 22:50:56]

Done.

+  }
+
+  bool IsJobControllerDeleted() {
+    return factory_->job_controller_set_.empty();
+  }
+
+  TestJobFactory job_factory_;
+  TestHttpStreamRequestDelegate request_delegate_;
+  SpdySessionDependencies session_deps_;
+  std::unique_ptr<HttpNetworkSession> session_;
+  HttpStreamFactoryImpl* factory_;
+  HttpStreamFactoryImpl::JobController* job_controller_;
+  std::unique_ptr<HttpStreamFactoryImpl::Request> request_;
+
+  DISALLOW_COPY_AND_ASSIGN(HttpStreamFactoryImplJobControllerTest);
+};
+
+INSTANTIATE_TEST_CASE_P(NextProto,
+                        HttpStreamFactoryImplJobControllerTest,
+                        testing::Values(kProtoSPDY31, kProtoHTTP2));
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       OnStreamFailedWithNoAlternativeJob) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+
+  EXPECT_TRUE(job_controller_->main_job());
+
+  // There's no other alternative job. Thus when stream failed, it should
+  // notify Request of the stream failure.
+  EXPECT_CALL(request_delegate_,
+              OnStreamFailed(ERR_FAILED, _, SSL_FAILURE_NONE))
+      .Times(1);
+  job_controller_->OnStreamFailed(job_factory_.main_job(), ERR_FAILED,
+                                  SSLConfig(), SSL_FAILURE_NONE);
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       OnStreamReadyWithNoAlternativeJob) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+
+  // There's no other alternative job. Thus when stream is ready, it should
+  // notify Request.
+  HttpStream* http_stream =
+      new HttpBasicStream(new ClientSocketHandle(), false);
+
+  SetStreamToJob(http_stream, job_factory_.main_job());
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
+      .WillOnce(Invoke(DeleteHttpStreamPointer));
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
+                                 ProxyInfo());
+}
+
+// Test we cancel Jobs correctly when the Request is explicitly canceled
+// before any Job is bound to Request.
+TEST_P(HttpStreamFactoryImplJobControllerTest, CancelJobsBeforeBinding) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(QUIC, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  // Reset the Request will cancel all the Jobs since there's no Job determined
+  // to serve Request yet and JobController will notify the factory to delete
+  // itself upon completion.
+  request_.reset();
+  EXPECT_TRUE(IsJobControllerDeleted());
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest, OnStreamFailedForBothJobs) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(QUIC, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  // We have |main_job| with unknown status when |alternative_job| is failed

[Ryan Hamilton, 2016/06/06 17:57:31]

nit: In the comments, instead of |main_job| and |alternative_job|, I'd just say
"the main job" and "the alternative job" since main_job isn't actually the name
of a variable.

[Zhongyi Shi, 2016/06/06 22:50:56]

Done.

+  // thus should not notify Request of |alternative_job|'s failure. But should
+  // notify |main_job| to mark |alternative_job| failed.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _, _)).Times(0);
+  EXPECT_CALL(*job_factory_.main_job(), MarkOtherJobComplete(_)).Times(1);
+  job_controller_->OnStreamFailed(job_factory_.alternative_job(), ERR_FAILED,
+                                  SSLConfig(), SSL_FAILURE_NONE);
+  EXPECT_TRUE(!job_controller_->alternative_job());
+  EXPECT_TRUE(job_controller_->main_job());
+
+  // The failure of second Job should be reported to Request as there's no more
+  // pending Job to serve the Request.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _, _)).Times(1);
+  job_controller_->OnStreamFailed(job_factory_.main_job(), ERR_FAILED,
+                                  SSLConfig(), SSL_FAILURE_NONE);

[Ryan Hamilton, 2016/06/06 17:57:31]

It would be good to use a different error code here than in the first failure
and to check for that error in the EXPECT_CALL.

[Zhongyi Shi, 2016/06/06 22:50:56]

Done.

+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       SecondJobFailsAfterFirstJobSucceeds) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(QUIC, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  // Main job succeeds, starts serving Request and it should report status
+  // to Request. The alternative job will mark the main job complete and gets
+  // orphaned.
+  HttpStream* http_stream =
+      new HttpBasicStream(new ClientSocketHandle(), false);
+  SetStreamToJob(http_stream, job_factory_.main_job());
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
+      .WillOnce(Invoke(DeleteHttpStreamPointer));
+  EXPECT_CALL(*job_factory_.alternative_job(), MarkOtherJobComplete(_))
+      .Times(1);
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
+                                 ProxyInfo());
+
+  // Reset the request as it's been successfully served.
+  request_.reset();

[Ryan Hamilton, 2016/06/06 17:57:31]

If request_ is reset, then there's no way the controller *could* notify the
request, right? which I think makes the comment below a bit moot. Does this test
still pass if the request is not reset?

[Zhongyi Shi, 2016/06/06 22:50:56]

The test will also pass if we don't reset the request_. I was mocking the
request is successfully served and gone. But it should be okay to have it served
but not reset, and the second job's failure should not be reported.

[Zhongyi Shi, 2016/06/06 22:50:56]

The test will also pass if we don't reset the request_. I was mocking the
request is successfully served and gone. But it should be okay to have it served
but not reset, and the second job's failure should not be reported.

+
+  // JobController shouldn't report the status of second job as request
+  // is gone served.
+  job_controller_->OnStreamFailed(job_factory_.alternative_job(), ERR_FAILED,
+                                  SSLConfig(), SSL_FAILURE_NONE);
+  EXPECT_TRUE(IsJobControllerDeleted());
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       SecondJobSucceedsAfterFirstJobFailed) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(QUIC, server.host(), 443);
+  SetAlternativeService(request_info, alternative_service);
+
+  request_.reset(
+      job_controller_->Start(request_info, &request_delegate_, nullptr,
+                             BoundNetLog(), HttpStreamRequest::HTTP_STREAM,
+                             DEFAULT_PRIORITY, SSLConfig(), SSLConfig()));
+  EXPECT_TRUE(job_controller_->main_job());
+  EXPECT_TRUE(job_controller_->alternative_job());
+
+  // |main_job| fails but should not report status to Request.
+  // The alternative job will mark the main job complete.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _, _)).Times(0);
+  EXPECT_CALL(*job_factory_.alternative_job(), MarkOtherJobComplete(_))
+      .Times(1);
+
+  job_controller_->OnStreamFailed(job_factory_.main_job(), ERR_FAILED,
+                                  SSLConfig(), SSL_FAILURE_NONE);
+
+  // |alternative_job| succeeds and should report status to Request.
+  HttpStream* http_stream =
+      new HttpBasicStream(new ClientSocketHandle(), false);
+  SetStreamToJob(http_stream, job_factory_.alternative_job());
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
+      .WillOnce(Invoke(DeleteHttpStreamPointer));
+  job_controller_->OnStreamReady(job_factory_.alternative_job(), SSLConfig(),
+                                 ProxyInfo());
+}

[Ryan Hamilton, 2016/06/06 17:57:31]

Woo hoo!

+
+}  // namespace net