Allow CertNetFetcher to be shutdown from the network thread

net/cert_net/cert_net_fetcher_impl.cc

Index: net/cert_net/cert_net_fetcher_impl.cc
diff --git a/net/cert_net/cert_net_fetcher_impl.cc b/net/cert_net/cert_net_fetcher_impl.cc
index bedd2af413bee6299b8c48e469830103ca155c47..8bb7181f97ae3300191c7a9b7cc595a0621bce56 100644
--- a/net/cert_net/cert_net_fetcher_impl.cc
+++ b/net/cert_net/cert_net_fetcher_impl.cc
@@ -13,34 +13,24 @@
 // caller can use to cancel the fetch, or wait for it to complete
 // (blocking).
 //
+// The CertNetFetcherImpl is shared between a network thread and a
+// caller thread that waits for fetches to happen on the network thread.
+//
 // The classes are mainly organized based on their thread affinity:
 //
 // ---------------
-// Lives on caller thread
+// Straddles caller thread and network thread
 // ---------------
 //
 // CertNetFetcherImpl (implements CertNetFetcher)
-//   * Main entry point
-//   * Provides a service to start/cancel/wait for URL fetches
+//   * Main entry point. Must be created and shutdown from the network thread.
+//   * Provides a service to start/cancel/wait for URL fetches, to be
+//     used on the caller thread.
 //   * Returns callers a CertNetFetcher::Request as a handle
 //   * Requests can run in parallel, however will block the current thread when
 //     reading results.
 //   * Posts tasks to network thread to coordinate actual work
 //
-// CertNetFetcherRequestImpl (implements CertNetFetcher::Request)
-//   * Wrapper for cancelling events, or waiting for a request to complete
-//   * Waits on a WaitableEvent to complete requests.
-//
-// ---------------
-// Straddles caller thread and network thread
-// ---------------
-//
-// CertNetFetcherCore
-//   * Reference-counted bridge between CertNetFetcherImpl and the dependencies
-//     on network thread.
-//   * Small wrapper to holds the state that is conceptually owned by
-//     CertNetFetcherImpl, but belongs on the network thread.
-//
 // RequestCore
 //   * Reference-counted bridge between CertNetFetcherRequestImpl and the
 //     dependencies on the network thread
@@ -48,6 +38,14 @@
 //     completion, and pointers for canceling work on network thread.
 //
 // ---------------
+// Lives on caller thread
+// ---------------
+//
+// CertNetFetcherRequestImpl (implements CertNetFetcher::Request)
+//   * Wrapper for cancelling events, or waiting for a request to complete
+//   * Waits on a WaitableEvent to complete requests.
+//
+// ---------------
 // Lives on network thread
 // ---------------
 //
@@ -69,12 +67,12 @@
 #include "base/memory/ptr_util.h"
 #include "base/numerics/safe_math.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/timer/timer.h"
 #include "net/base/load_flags.h"
 #include "net/cert/cert_net_fetcher.h"
 #include "net/url_request/redirect_info.h"
 #include "net/url_request/url_request_context.h"
-#include "net/url_request/url_request_context_getter.h"
 
 // TODO(eroman): Add support for POST parameters.
 // TODO(eroman): Add controls for bypassing the cache.
@@ -121,21 +119,25 @@ using JobSet = std::map<Job*, std::unique_ptr<Job>, JobComparator>;
 class AsyncCertNetFetcherImpl {
  public:
   // Initializes AsyncCertNetFetcherImpl using the specified URLRequestContext
-  // for issuing requests. |context| must remain valid for the entire
-  // lifetime of the AsyncCertNetFetcherImpl.
+  // for issuing requests. |context| must remain valid until Shutdown() is
+  // called or the AsyncCertNetFetcherImpl is destroyed.
   explicit AsyncCertNetFetcherImpl(URLRequestContext* context);
 
-  // Deletion implicitly cancels any outstanding requests.
+  // The AsyncCertNetFetcherImpl is expected to be kept alive until all
+  // requests have completed or Shutdown() is called.
   ~AsyncCertNetFetcherImpl();
 
   // Starts an asynchronous request to fetch the given URL. On completion
-  // |callback| will be invoked.
+  // request->OnJobCompleted() will be invoked.

[eroman, 2017/01/11 01:56:59]

Thanks for fixing

   //
-  // Completion of the request will never occur synchronously. In other words it
-  // is guaranteed that |callback| will only be invoked once the Fetch*() method
-  // has returned.
+  // Completion of the request will never occur synchronously. In other
+  // words it is guaranteed that request->OnJobCompleted() will only be

[eroman, 2017/01/11 01:56:59]

Can you remove the comment about guaranteeing OnJobCompleted() is not called
synchronously?

That behavior was a carry-over from the callback-based approach and isn't needed
anymore.

By removing that guartantee we should be able to simplify this change.

[estark, 2017/01/12 01:06:30]

Done.

+  // invoked once the Fetch*() method has returned.
   void Fetch(std::unique_ptr<RequestParams> request_params,
-             RequestCore* request);
+             scoped_refptr<RequestCore> request);
+
+  // Cancels outstanding jobs.
+  void Shutdown();
 
  private:
   friend class Job;
@@ -225,6 +227,10 @@ class RequestCore : public base::RefCountedThreadSafe<RequestCore> {
   // Can be called from any thread.
   void Cancel();
 
+  // Signals that an error was encountered before the request was
+  // attached to a job. Should only be called from the caller thread.
+  void SignalImmediateError();
+
   // Should only be called once.
   void WaitForResult(Error* error, std::vector<uint8_t>* bytes) {
     DCHECK(!task_runner_->RunsTasksOnCurrentThread());
@@ -297,9 +303,8 @@ class Job : public URLRequest::Delegate {
   const RequestParams& request_params() const { return *request_params_; }
 
   // Create a request and attaches it to the job. When the job completes it will

[eroman, 2017/01/11 01:56:59]

side-comment: Could you update Create --> Creates ?

[estark, 2017/01/12 01:06:30]

Done.

-  // notify the request of completion through OnJobCompleted. Note that the Job
-  // does NOT own the request.
-  void AttachRequest(RequestCore* request);
+  // notify the request of completion through OnJobCompleted.
+  void AttachRequest(scoped_refptr<RequestCore> request);
 
   // Removes |request| from the job.
   void DetachRequest(RequestCore* request);
@@ -309,6 +314,12 @@ class Job : public URLRequest::Delegate {
   // notified of completion.
   void StartURLRequest(URLRequestContext* context);
 
+  // Cancels the request with an ERR_ABORTED error and invokes
+  // OnJobCompleted() to notify the registered requests of the
+  // cancellation. If StartURLRequest() is invoked subsequently, it will

[eroman, 2017/01/11 01:57:00]

See earlier comment.

We should be able to call r->OnJobCompleted() immediately without needing to use
a deferred task.

[estark, 2017/01/12 01:06:30]

Addressed the earlier comment, but I don't think I see what needs to be changed
here?

+  // immediately call OnJobCompleted() with an error.
+  void Cancel();
+
  private:
   // Implementation of URLRequest::Delegate
   void OnReceivedRedirect(URLRequest* request,
@@ -340,8 +351,8 @@ class Job : public URLRequest::Delegate {
   // OnUrlRequestCompleted().
   void FailRequest(Error error);
 
-  // The requests attached to this job (non-owned).
-  std::vector<RequestCore*> requests_;
+  // The requests attached to this job.
+  std::vector<scoped_refptr<RequestCore>> requests_;
 
   // The input parameters for starting a URLRequest.
   std::unique_ptr<RequestParams> request_params_;
@@ -359,6 +370,10 @@ class Job : public URLRequest::Delegate {
   // Non-owned pointer to the AsyncCertNetFetcherImpl that created this job.
   AsyncCertNetFetcherImpl* parent_;
 
+  // When set, StartURLRequest() will post a task to run
+  // OnJobCompleted() with an error and return immediately.
+  bool cancelled_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
@@ -375,7 +390,13 @@ void RequestCore::Cancel() {
   }
 
   bytes_.clear();
-  error_ = ERR_UNEXPECTED;
+  error_ = ERR_ABORTED;
+}
+
+void RequestCore::SignalImmediateError() {
+  DCHECK(!task_runner_->RunsTasksOnCurrentThread());
+  error_ = ERR_ABORTED;
+  completion_event_.Signal();
 }
 
 Job::Job(std::unique_ptr<RequestParams> request_params,
@@ -387,7 +408,7 @@ Job::~Job() {
   Stop();
 }
 
-void Job::AttachRequest(RequestCore* request) {
+void Job::AttachRequest(scoped_refptr<RequestCore> request) {
   requests_.push_back(request);

[eroman, 2017/01/11 01:56:59]

std::move()

[estark, 2017/01/12 01:06:30]

Done.

   request->AttachedToJob(this);
 }
@@ -406,6 +427,12 @@ void Job::DetachRequest(RequestCore* request) {
 }
 
 void Job::StartURLRequest(URLRequestContext* context) {
+  if (cancelled_) {

[eroman, 2017/01/11 01:56:59]

I don't think we need this boolean.

AsyncCertNetFetcherImpl::Shutdown() should call Cancel() on each job (which
informs each RequestCore of completion), and then delete the Job.

So we shouldn't end up calling StartURLRequest after shutdown/cancellation
right?

[estark, 2017/01/12 01:06:30]

Ah, yes, that's right, thanks. Done.

+    timer_.Start(
+        FROM_HERE, base::TimeDelta(),
+        base::Bind(&Job::OnJobCompleted, base::Unretained(this), ERR_ABORTED));
+    return;
+  }
   Error error = CanFetchUrl(request_params_->url);
   if (error != OK) {
     // TODO(eroman): Don't post a task for this case.

[eroman, 2017/01/11 01:57:00]

[optional] While you are here, you could also remove this post task (shouldn't
need to post a task for this; previously the concern was not invoking the
completion callback synchronously, but that is no longer the design we have).

[estark, 2017/01/12 01:06:30]

Done.

@@ -432,6 +459,11 @@ void Job::StartURLRequest(URLRequestContext* context) {
         base::Bind(&Job::FailRequest, base::Unretained(this), ERR_TIMED_OUT));
 }
 
+void Job::Cancel() {
+  cancelled_ = true;
+  FailRequest(ERR_ABORTED);
+}
+
 void Job::OnReceivedRedirect(URLRequest* request,
                              const RedirectInfo& redirect_info,
                              bool* defer_redirect) {
@@ -525,7 +557,7 @@ void Job::OnJobCompleted(Error error) {
 
   std::unique_ptr<Job> delete_this = parent_->RemoveJob(this);
 
-  for (auto* request : requests_) {
+  for (auto request : requests_) {

[eroman, 2017/01/11 01:56:59]

nit: remove the spurious reference counting during this loop

[estark, 2017/01/12 01:06:31]

Done. (I think -- this is done via const auto&, correct?)

     request->OnJobCompleted(this, error, response_body_);
   }
 
@@ -555,7 +587,7 @@ bool JobComparator::operator()(const Job* job1, const Job* job2) const {
 
 void AsyncCertNetFetcherImpl::Fetch(
     std::unique_ptr<RequestParams> request_params,
-    RequestCore* request) {
+    scoped_refptr<RequestCore> request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // If there is an in-progress job that matches the request parameters use it.
@@ -571,6 +603,18 @@ void AsyncCertNetFetcherImpl::Fetch(
   return job->AttachRequest(request);
 }
 
+void AsyncCertNetFetcherImpl::Shutdown() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  for (JobSet::iterator job = jobs_.begin(); job != jobs_.end();) {

[eroman, 2017/01/11 01:56:59]

Does post-increment work?

for (auto it = jobs_.begin(); it != jobs_.end(); ) {
  // Canceling the job also deletes it.
  (it++)->Cancel();
}

That said this is pretty fragile and I don't think we should structure it that
way.

Instead, how about making Cancel() just cancel the job but not delete it?

(then shutdown can bulk clear the jobs after the Cancel() loop).

How about instead changing Cancel() so that it does not delete.

Maybe something 
Could you change Cancel() to not delete itself?

[estark, 2017/01/12 01:06:30]

Done.

+    // Jobs get removed when they are cancelled, so increment before
+    // the |job| iterator is erased.
+    JobSet::iterator next = job;
+    ++next;
+    job->first->Cancel();
+    job = next;
+  }
+}
+
 struct JobToRequestParamsComparator {
   bool operator()(const JobSet::value_type& job,
                   const RequestParams& value) const {
@@ -622,60 +666,22 @@ class CertNetFetcherRequestImpl : public CertNetFetcher::Request {
   scoped_refptr<RequestCore> core_;
 };
 
-class CertNetFetcherCore
-    : public base::RefCountedThreadSafe<CertNetFetcherCore> {
+class CertNetFetcherImpl : public CertNetFetcher {
  public:
-  explicit CertNetFetcherCore(URLRequestContextGetter* context_getter)
-      : context_getter_(context_getter) {}
-
-  void Abandon() {
-    GetNetworkTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&CertNetFetcherCore::DoAbandonOnNetworkThread, this));
-  }
+  explicit CertNetFetcherImpl(URLRequestContext* context)
+      : task_runner_(base::ThreadTaskRunnerHandle::Get()), context_(context) {}
 
-  scoped_refptr<base::SingleThreadTaskRunner> GetNetworkTaskRunner() {
-    return context_getter_->GetNetworkTaskRunner();
-  }
-
-  void DoFetchOnNetworkThread(std::unique_ptr<RequestParams> request_params,
-                              scoped_refptr<RequestCore> request) {
-    DCHECK(GetNetworkTaskRunner()->RunsTasksOnCurrentThread());
-
-    if (!impl_) {
-      impl_.reset(
-          new AsyncCertNetFetcherImpl(context_getter_->GetURLRequestContext()));
+  void Shutdown() override {
+    DCHECK(task_runner_->RunsTasksOnCurrentThread());
+    base::AutoLock auto_lock(shutdown_lock_);
+    shutdown_ = true;
+    if (impl_) {
+      impl_->Shutdown();
+      impl_.reset();
     }
-
-    // Don't need to retain a reference to |request| because consume is
-    // expected to keep it alive.
-    impl_->Fetch(std::move(request_params), request.get());
-  }
-
- private:
-  friend class base::RefCountedThreadSafe<CertNetFetcherCore>;
-
-  void DoAbandonOnNetworkThread() {
-    DCHECK(GetNetworkTaskRunner()->RunsTasksOnCurrentThread());
-    impl_.reset();
+    context_ = nullptr;
   }
 
-  ~CertNetFetcherCore() { DCHECK(!impl_); }
-
-  scoped_refptr<URLRequestContextGetter> context_getter_;
-
-  std::unique_ptr<AsyncCertNetFetcherImpl> impl_;
-
-  DISALLOW_COPY_AND_ASSIGN(CertNetFetcherCore);
-};
-
-class CertNetFetcherImpl : public CertNetFetcher {
- public:
-  explicit CertNetFetcherImpl(URLRequestContextGetter* context_getter)
-      : core_(new CertNetFetcherCore(context_getter)) {}
-
-  ~CertNetFetcherImpl() override { core_->Abandon(); }
-
   std::unique_ptr<Request> FetchCaIssuers(const GURL& url,
                                           int timeout_milliseconds,
                                           int max_response_bytes) override {
@@ -720,28 +726,53 @@ class CertNetFetcherImpl : public CertNetFetcher {
   }
 
  private:
+  ~CertNetFetcherImpl() override {}

[eroman, 2017/01/11 01:56:59]

From what I can tell this relies on Shutdown() having already been called
(meaning the contract is Shutdown() is ALWAYS called.

Please add an assertion to verify this.

[estark, 2017/01/12 01:06:31]

Done.

(This invalidated one of the tests I had, which was stopping the network thread
before shutting down the fetcher. That would now violate the contract that
Shutdown must be called before the fetcher is destroyed, so I removed that
test.)

+
+  void DoFetchOnNetworkThread(std::unique_ptr<RequestParams> request_params,
+                              scoped_refptr<RequestCore> request) {
+    DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+    if (!context_) {
+      request->Cancel();

[eroman, 2017/01/11 01:56:59]

I don't think this is right.

RequestCore::Cancel()  cancels any outstanding URLRequest (of which there is
none yet), and DOES NOT notify  the client waiting on the request.

I believe this needs something akin to SignalImmediateError() instead.

[estark, 2017/01/12 01:06:31]

Ah, oops, that was indeed supposed to be SignalImmediateError. Fixed, and I
attempted to write a test for this but I'm not sure if I did something
reasonable; see comment in the unit tests.

[estark, 2017/01/12 01:19:10]

Sorry, this comment about the test is out-of-date. Since I removed the
|shutdown_| flag, this case is now tested by the test that shuts down the
fetcher before starting the request, so I didn't need an additional test for it.

+      return;
+    }
+
+    if (!impl_) {
+      impl_.reset(new AsyncCertNetFetcherImpl(context_));
+    }
+
+    impl_->Fetch(std::move(request_params), request);
+  }
+
   std::unique_ptr<Request> DoFetch(
       std::unique_ptr<RequestParams> request_params) {
-    auto task_runner = core_->GetNetworkTaskRunner();
-    scoped_refptr<RequestCore> request_core = new RequestCore(task_runner);
-
-    task_runner->PostTask(
-        FROM_HERE,
-        base::Bind(&CertNetFetcherCore::DoFetchOnNetworkThread, core_,
-                   base::Passed(&request_params), request_core));
+    scoped_refptr<RequestCore> request_core = new RequestCore(task_runner_);
+
+    base::AutoLock auto_lock(shutdown_lock_);

[eroman, 2017/01/11 01:56:59]

Are you locking here to better handle the case where the thread identified by
task_runner_ (the network thread) is stopped?

Because otherwise posting a task and then checking cancellation while on the
Network thread (without a lock) is simpler.

(I wonder if that case is encompassed by PostTask() returning false?)

Either way, please add comments.

[estark, 2017/01/12 01:06:30]

Yes, I was thinking of that case, because I can't convince myself that we can
ever rely on PostTask returning false. But, since we aren't in general
guaranteeing that requests ever complete, I guess it's simpler to not try to
handle this case specially, so I removed the |shutdown_| flag and lock and added
a comment.

+    if (shutdown_ ||
+        !task_runner_->PostTask(
+            FROM_HERE,
+            base::Bind(&CertNetFetcherImpl::DoFetchOnNetworkThread, this,
+                       base::Passed(&request_params), request_core))) {
+      request_core->SignalImmediateError();
+    }
 
     return base::MakeUnique<CertNetFetcherRequestImpl>(std::move(request_core));
   }
 
  private:
-  scoped_refptr<CertNetFetcherCore> core_;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  // Not owned. |context_| must stay valid until Shutdown() is called.
+  URLRequestContext* context_ = nullptr;
+  std::unique_ptr<AsyncCertNetFetcherImpl> impl_;
+  base::Lock shutdown_lock_;
+  bool shutdown_ = false;
 };
 
 }  // namespace
 
-std::unique_ptr<CertNetFetcher> CreateCertNetFetcher(
-    URLRequestContextGetter* context_getter) {
-  return base::MakeUnique<CertNetFetcherImpl>(context_getter);
+scoped_refptr<CertNetFetcher> CreateCertNetFetcher(URLRequestContext* context) {
+  return make_scoped_refptr(new CertNetFetcherImpl(context));
 }
 
 }  // namespace net