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..ca8b4feb6ae7bbdde6e196849f465b0c15585cab
--- /dev/null
+++ b/net/http/http_stream_factory_impl_job_controller_unittest.cc
@@ -0,0 +1,313 @@
+// 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::_;
+
+namespace net {
+
+class HttpStreamFactoryImplJobControllerTest
+    : public ::testing::Test,
+      public ::testing::WithParamInterface<NextProto> {
+ public:
+  HttpStreamFactoryImplJobControllerTest()
+      : job_controller_peer_(
+            new TestJobControllerPeer(GetParam(), &request_delegate_)) {}

[Ryan Hamilton, 2016/06/01 18:43:07]

where is TestJobControllerPeer defined?

[Zhongyi Shi, 2016/06/02 20:49:09]

Oh, it's defined in test_util.h, but I have moved it here now.

+
+  ~HttpStreamFactoryImplJobControllerTest() {}
+
+  HttpStreamFactoryImpl::JobController* job_controller() {
+    return job_controller_peer_->job_controller();
+  }
+
+  std::unique_ptr<TestJobControllerPeer> job_controller_peer_;

[Ryan Hamilton, 2016/06/01 18:43:07]

Instead of a unique_ptr, can you just make this a non-pointer member?

[Zhongyi Shi, 2016/06/02 20:49:08]

Done.

+  TestHttpStreamRequestDelegate request_delegate_;
+};
+
+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");
+  job_controller_peer_->CreateRequest(request_info);
+
+  HostPortPair server = HostPortPair::FromURL(request_info.url);
+  GURL original_url =
+      job_controller_peer_->ApplyHostMappingRules(request_info.url, &server);
+
+  TestHttpStreamFactoryImplJob* main_job = new TestHttpStreamFactoryImplJob(
+      job_controller(), HttpStreamFactoryImpl::MAIN,
+      job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+      SSLConfig(), SSLConfig(), server, original_url, nullptr);
+  job_controller_peer_->SetMainJob(main_job);

[Ryan Hamilton, 2016/06/01 18:43:07]

It would be better if we didn't have to use a peer here. The controller could
take a JobFactory which would normally create a Job*, but the test could pass in
a factory which returned a TestJob*.

[Zhongyi Shi, 2016/06/02 20:49:09]

I added a JobFactory to impl and pass that in JobController,
JobFactory::CreateJob() will create HttpStreamFactoryImpl::Job for controller.

When testing, we need to call AddMainJobToFactory which will returns a TestJob*
to the test for mock, and add that to TestJobFactory. When requested for Job,
TestJobFactory will return TestJob to controller so that we can easily set up
mock expectations. Does this make sense?

+
+  // There's no bound job yet, and no other alternative job. Thus when stream
+  // failed, it should call BindJob to bind this Job. And then notify Request
+  // of the stream failure.

[Ryan Hamilton, 2016/06/01 18:43:07]

I think you can remove references to binding the job (and the tests too) since
that's not visible via the public interfaces.

[Zhongyi Shi, 2016/06/02 20:49:08]

All the references to bound_job and job_bound are removed as we could deduce the
status from whether request is notified by the job: if notified by |job|, the
request is bound with |job|. Otherwise, not bound.

+  EXPECT_CALL(request_delegate_,
+              OnStreamFailed(ERR_FAILED, _, SSL_FAILURE_NONE))
+      .Times(1);
+
+  job_controller_peer_->OnStreamFailed(main_job, ERR_FAILED, SSLConfig(),
+                                       SSL_FAILURE_NONE);
+  EXPECT_TRUE(job_controller_peer_->job_bound());
+  EXPECT_EQ(job_controller_peer_->bound_job(), main_job);
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       OnStreamReadyWithNoAlternativeJob) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+  job_controller_peer_->CreateRequest(request_info);
+
+  HostPortPair server = HostPortPair::FromURL(request_info.url);
+  GURL original_url =
+      job_controller_peer_->ApplyHostMappingRules(request_info.url, &server);
+
+  TestHttpStreamFactoryImplJob* main_job = new TestHttpStreamFactoryImplJob(
+      job_controller(), HttpStreamFactoryImpl::MAIN,
+      job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+      SSLConfig(), SSLConfig(), server, original_url, nullptr);
+  job_controller_peer_->SetMainJob(main_job);
+
+  // There's no bound job yet, and no other alternative job. Thus when stream
+  // is ready, it should call BindJob to bind this Job. And then notify Request.
+  HttpStream* http_stream =
+      new HttpBasicStream(new ClientSocketHandle(), false);
+  main_job->SetStream(http_stream);
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, _)).Times(1);
+  job_controller_peer_->OnStreamReady(main_job, SSLConfig(), ProxyInfo());
+  EXPECT_TRUE(job_controller_peer_->job_bound());
+  EXPECT_EQ(job_controller_peer_->bound_job(), main_job);
+}
+
+// Test we cancel Job(s) 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("http://www.google.com");
+
+  job_controller_peer_->CreateRequest(request_info);
+
+  HostPortPair server = HostPortPair::FromURL(request_info.url);
+  GURL original_url =
+      job_controller_peer_->ApplyHostMappingRules(request_info.url, &server);
+
+  TestHttpStreamFactoryImplJob* main_job = new TestHttpStreamFactoryImplJob(
+      job_controller(), HttpStreamFactoryImpl::MAIN,
+      job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+      SSLConfig(), SSLConfig(), server, original_url, nullptr);
+  job_controller_peer_->SetMainJob(main_job);
+
+  AlternativeService alternative_service(net::NPN_HTTP_2, server);
+  TestHttpStreamFactoryImplJob* alternative_job =
+      new TestHttpStreamFactoryImplJob(
+          job_controller(), HttpStreamFactoryImpl::ALTERNATIVE,
+          job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+          SSLConfig(), SSLConfig(), server, original_url, alternative_service,
+          nullptr);
+  job_controller_peer_->SetAlternativeJob(alternative_job);
+
+  EXPECT_FALSE(job_controller_peer_->job_bound());
+  // Reset the Request will cancel all the Job(s) since there's no Job bound
+  // yet and JobController will notify the factory to delete the it upon
+  // upon completion.
+  job_controller_peer_->CancelRequest();
+  EXPECT_TRUE(job_controller_peer_->IsJobControllerDeleted());
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest, OnStreamFailedForBothJobs) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+
+  job_controller_peer_->CreateRequest(request_info);
+
+  HostPortPair server = HostPortPair::FromURL(request_info.url);
+  GURL original_url =
+      job_controller_peer_->ApplyHostMappingRules(request_info.url, &server);
+
+  TestHttpStreamFactoryImplJob* main_job = new TestHttpStreamFactoryImplJob(
+      job_controller(), HttpStreamFactoryImpl::MAIN,
+      job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+      SSLConfig(), SSLConfig(), server, original_url, nullptr);
+  job_controller_peer_->SetMainJob(main_job);
+
+  AlternativeService alternative_service(net::NPN_HTTP_2, server);
+  TestHttpStreamFactoryImplJob* alternative_job =
+      new TestHttpStreamFactoryImplJob(
+          job_controller(), HttpStreamFactoryImpl::ALTERNATIVE,
+          job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+          SSLConfig(), SSLConfig(), server, original_url, alternative_service,
+          nullptr);
+  job_controller_peer_->SetAlternativeJob(alternative_job);
+
+  // We have |main_job| with unknown status when |alternative_job| is failed
+  // thus should not bind Job or notify Request of |alternative_job|'s status.
+  // But should notify |main_job| to mark |alternative_job| failed.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _, _)).Times(0);
+  EXPECT_CALL(*main_job, MarkOtherJobComplete(_)).Times(1);
+  job_controller_peer_->OnStreamFailed(alternative_job, ERR_FAILED, SSLConfig(),
+                                       SSL_FAILURE_NONE);
+  EXPECT_TRUE(!job_controller_peer_->job_bound());
+  EXPECT_TRUE(!job_controller_peer_->bound_job());
+  EXPECT_TRUE(!job_controller_peer_->alternative_job());
+  EXPECT_TRUE(job_controller_peer_->main_job());
+
+  // The failure of second Job should be reported to Request as there's no more
+  // pending Job to server the Request.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _, _)).Times(1);
+  job_controller_peer_->OnStreamFailed(main_job, ERR_FAILED, SSLConfig(),
+                                       SSL_FAILURE_NONE);

[Ryan Hamilton, 2016/06/01 18:43:07]

nice!

+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       SecondJobFailsAfterFirstJobSucceeds) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+  job_controller_peer_->CreateRequest(request_info);
+
+  HostPortPair server = HostPortPair::FromURL(request_info.url);
+  GURL original_url =
+      job_controller_peer_->ApplyHostMappingRules(request_info.url, &server);
+
+  TestHttpStreamFactoryImplJob* main_job = new TestHttpStreamFactoryImplJob(
+      job_controller(), HttpStreamFactoryImpl::MAIN,
+      job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+      SSLConfig(), SSLConfig(), server, original_url, nullptr);
+  job_controller_peer_->SetMainJob(main_job);
+
+  AlternativeService alternative_service(net::NPN_HTTP_2, server);
+  TestHttpStreamFactoryImplJob* alternative_job =
+      new TestHttpStreamFactoryImplJob(
+          job_controller(), HttpStreamFactoryImpl::ALTERNATIVE,
+          job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+          SSLConfig(), SSLConfig(), server, original_url, alternative_service,
+          nullptr);
+  job_controller_peer_->SetAlternativeJob(alternative_job);
+
+  // Main job succeeds and gets bound to 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);
+  main_job->SetStream(http_stream);
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, _)).Times(1);
+  EXPECT_CALL(*alternative_job, MarkOtherJobComplete(_)).Times(1);
+  job_controller_peer_->OnStreamReady(main_job, SSLConfig(), ProxyInfo());
+  EXPECT_TRUE(job_controller_peer_->job_bound());
+  EXPECT_EQ(job_controller_peer_->bound_job(), main_job);
+
+  // Reset the request as it's been successfully served.
+  job_controller_peer_->CancelRequest();
+  EXPECT_TRUE(job_controller_peer_->job_bound());
+  EXPECT_EQ(job_controller_peer_->bound_job(), nullptr);
+
+  // Though bound_job_ is cleared, but JobController knows a job has ever been
+  // bound. JobController shouldn't report the status of second job as request
+  // is gone served.
+  job_controller_peer_->OnStreamFailed(alternative_job, ERR_FAILED, SSLConfig(),
+                                       SSL_FAILURE_NONE);
+  EXPECT_TRUE(job_controller_peer_->IsJobControllerDeleted());
+  delete http_stream;
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest,
+       SecondJobSucceedsAfterFirstJobFailed) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+  job_controller_peer_->CreateRequest(request_info);
+
+  HostPortPair server = HostPortPair::FromURL(request_info.url);
+  GURL original_url =
+      job_controller_peer_->ApplyHostMappingRules(request_info.url, &server);
+
+  TestHttpStreamFactoryImplJob* main_job = new TestHttpStreamFactoryImplJob(
+      job_controller(), HttpStreamFactoryImpl::MAIN,
+      job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+      SSLConfig(), SSLConfig(), server, original_url, nullptr);
+  job_controller_peer_->SetMainJob(main_job);
+
+  AlternativeService alternative_service(net::NPN_HTTP_2, server);
+  TestHttpStreamFactoryImplJob* alternative_job =
+      new TestHttpStreamFactoryImplJob(
+          job_controller(), HttpStreamFactoryImpl::ALTERNATIVE,
+          job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+          SSLConfig(), SSLConfig(), server, original_url, alternative_service,
+          nullptr);
+  job_controller_peer_->SetAlternativeJob(alternative_job);
+
+  // |main_job| fails but should not get bound and report status to Request.
+  // The alternative job will mark the main job complete and gets orphaned.
+  EXPECT_CALL(request_delegate_, OnStreamFailed(_, _, _)).Times(0);
+  EXPECT_CALL(*alternative_job, MarkOtherJobComplete(_)).Times(1);
+
+  job_controller_peer_->OnStreamFailed(main_job, ERR_FAILED, SSLConfig(),
+                                       SSL_FAILURE_NONE);
+  EXPECT_FALSE(job_controller_peer_->job_bound());
+  EXPECT_EQ(job_controller_peer_->bound_job(), nullptr);
+
+  // |alternative_job| succeeds and should report status to Request.
+  HttpStream* http_stream =
+      new HttpBasicStream(new ClientSocketHandle(), false);
+  alternative_job->SetStream(http_stream);
+
+  EXPECT_CALL(request_delegate_, OnStreamReady(_, _, _)).Times(1);
+  job_controller_peer_->OnStreamReady(alternative_job, SSLConfig(),
+                                      ProxyInfo());
+  EXPECT_EQ(job_controller_peer_->bound_job(), alternative_job);
+
+  delete http_stream;
+}
+
+TEST_P(HttpStreamFactoryImplJobControllerTest, NotifyFactoryOfCompletion) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+  HostPortPair server = HostPortPair::FromURL(request_info.url);
+  GURL original_url =
+      job_controller_peer_->ApplyHostMappingRules(request_info.url, &server);
+
+  TestHttpStreamFactoryImplJob* main_job = new TestHttpStreamFactoryImplJob(
+      job_controller(), HttpStreamFactoryImpl::MAIN,
+      job_controller_peer_->session(), request_info, DEFAULT_PRIORITY,
+      SSLConfig(), SSLConfig(), server, original_url, nullptr);
+  job_controller_peer_->SetMainJob(main_job);
+
+  // HttpStreamFactoryImpl shouldn't be notified to delete the JobController
+  // when there's still Job left.
+  job_controller_peer_->MaybeNotifyFactoryOfCompletion();
+  EXPECT_FALSE(job_controller_peer_->IsJobControllerDeleted());
+
+  // HttpStreamFactoryImpl should be notified to delete the JobController
+  // when there's no Job as well as Request left.
+  job_controller_peer_->ClearMainJob();
+  job_controller_peer_->MaybeNotifyFactoryOfCompletion();
+  EXPECT_TRUE(job_controller_peer_->IsJobControllerDeleted());
+}
+

[Zhongyi Shi, 2016/06/02 20:49:08]

This test is removed as MaybeNotifyFactoryOfCompletion is private method, and
it's tested inherently in other tests where we checked IsJobControllerDeleted().

+}  // namespace net