Create base::WorkerPoolJob. Use it for HostResolverImpl and DirectoryLister.

base/worker_pool_job.h

+// Copyright (c) 2010 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.
+
+#ifndef BASE_WORKER_POOL_JOB_H_
+#define BASE_WORKER_POOL_JOB_H_
+#pragma once
+
+#include "base/basictypes.h"
+#include "base/lock.h"
+#include "base/message_loop_proxy.h"
+#include "base/ref_counted.h"
+#include "base/thread_checker.h"
+
+class MessageLoop;
+
+namespace base {
+
+// WARNING: Are you sure you want to use WorkerPool in the first place?  The
+// thread will _not_ be joined, so the job may still be running during shutdown.
+// Global objects that are deleted by AtExitManager should not be accessed on
+// the WorkerPool thread, otherwise there is potential for shutdown crashes.
+//
+// WorkerPoolJob is a helper class for implementing jobs that get created on an
+// origin thread, get posted to run on a WorkerPool thread, and then run back on
+// the origin thread upon completion.  WorkerPoolJob also handles cancellation
+// of a job which prevents it from calling back.  Note that we use inheritance
+// rather than composition here, since WorkerPoolJob also handles lifetime
+// management via refcounting.
+class WorkerPoolJob : public base::RefCountedThreadSafe<WorkerPoolJob> {
+ public:
+  // ScopedHandle is provided to automate cancellation of the WorkerPoolJob.
+  template <typename JobType>

[eroman, 2010/12/22 00:51:47]

Why is this templatized? 

Since this is already a base class we should be able to access the protected
members regardless of the derived type.

[willchan no longer on Chromium, 2010/12/22 02:04:01]

The job() accessor needs the templated param to control the return type.

+  class ScopedHandle {
+   public:
+    explicit ScopedHandle(JobType* job) : job_(job) {}
+
+    ~ScopedHandle() {
+      AutoLock auto_lock(job_->lock_);
+      if (!job_->canceled_ && job_->state_ > NONE &&
+          job_->state_ < CANCELED_ON_WORKER)
+        job_->canceled_ = true;
+    }
+
+    JobType* job() const { return job_.get(); }
+
+   private:
+    const scoped_refptr<JobType> job_;
+    DISALLOW_COPY_AND_ASSIGN(ScopedHandle);
+  };
+
+ protected:
+  explicit WorkerPoolJob(bool is_slow);

[eroman, 2010/12/22 00:51:47]

I think we should get rid of this bool.

No consumer of the workerpool ever specifies anything other than true. And in
fact the boolean is disregarded everywhere except windows.

It makes sense for this to only ever be true, since by definition we wouldn't be
using the worker pool for "fast" jobs.

[willchan no longer on Chromium, 2010/12/22 02:04:01]

Done.

+  virtual ~WorkerPoolJob();

[eroman, 2010/12/22 00:51:47]

Please stress somewhere that subclasses MUST be prepared to be deleted on either
the origin thread, or the worker thread.

[willchan no longer on Chromium, 2010/12/22 02:04:01]

Done.

+
+  // Starts the job.  Will post itself to the WorkerPool, where it will invoke
+  // RunJob().  After RunJob() runs, it will post itself back to the origin
+  // thread, where it will invoke CompleteJob().  This can only be called once
+  // and must be called on the origin thread.
+  void StartJob();
+
+  // Cancels the job.  CompleteJob() is guaranteed not to be called when this
+  // happens.  RunJob() may or may not be called, depending on when CancelJob()
+  // is called.  it's fundamentally a race.  When cancellation is detected (may

[eroman, 2010/12/22 00:51:47]

it's --> It's

[willchan no longer on Chromium, 2010/12/22 02:04:01]

Done.

+  // be on the WorkerPool thread or the origin thread), OnCanceled() is invoked.

[eroman, 2010/12/22 00:51:47]

Perhaps a more succinct description is to say that we are guaranteed
CompleteJob() will never be called after CancelJob() has been called.

Seems like this isn't currently true, but see my rant below about simplifying
the cancellation semantics.

[willchan no longer on Chromium, 2010/12/22 02:04:01]

I'm going to wait until we settle the cancellation semantics.  It's been awhile
since I looked at this, but I grudgingly added OnCanceled() to make the tests
work.

+  // This can only be called once and must be called on the origin thread.  It
+  // should only be called after StartJob() is called and before
+  // CompleteJob() is called (if it is called).
+  void CancelJob();
+
+  // Returns true if CancelJob() has been called.
+  bool canceled() const;
+
+  // Returns true if the job is running on the worker thread.  Obviously if it
+  // returns true once, there's still no guarantee that it is still running
+  // after it returns.  Once it returns false though, then it's guaranteed to
+  // stay false.  is_running() is only allowed to be called on the origin
+  // thread.
+  bool is_running() const;
+
+  // NOTE(willchan): I've restricted this to unit tests only, primarily because
+  // I haven't found a good reason for allowing subtypes to access this.  If
+  // there is a motivating reason to do so, I am open to it.
+#if defined(UNIT_TEST)
+  scoped_refptr<MessageLoopProxy> origin_loop() const {
+    return origin_loop_;
+  }
+#endif  // defined(UNIT_TEST)
+
+ private:
+  friend class base::RefCountedThreadSafe<WorkerPoolJob>;
+  template <typename JobType> friend class ScopedHandle;
+
+  // State machine, primarily for enforcing that certain events only happen
+  // once.  Also lets us implement is_running().
+  enum State {
+    NONE,
+    RUNNING,
+    FINISHING,
+    CANCELED_ON_WORKER,
+    DONE,
+  };
+
+  // API for subtypes to implement.
+
+  // RunJob() runs on the worker thread.  If CancelJob() is called on the origin
+  // thread , then it may never be invoked, but that's a race.
+  virtual void RunJob() = 0;

[eroman, 2010/12/22 00:51:47]

Listing pure virtual methods under private section seems weird to me. I would
put them under at least "protected".

[willchan no longer on Chromium, 2010/12/22 02:04:01]

No, it's more correct to make them private.  That means only the base type can
call the virtual.  protected would allow subtypes to also call the member
function.  Allowing the subtype to call RunJob() would be a bug.

+
+  // CompleteJob() runs on the origin thread.  If CancelJob() is called, then it
+  // may never be invoked.
+  virtual void CompleteJob() = 0;
+
+  // OnCanceled() may be called on either the WorkerPool thread or the origin
+  // thread.
+  virtual void OnCanceled() {}

[eroman, 2010/12/22 00:51:47]

Please, no.

First of all, I don't like default virtual method implementations, they can
cause problems.

Secondly, I don't think it makes sense to have a cancellation callback.

The *client* is the one calling Cancel(), as a way to tell the job to not notify
them of completion. It seems overly complicated to have this funky OnCancelled()
callback that can be called from either the worker thread, or the origin loop.

If the client cares about running other stuff on cancellation, they can very
well just call another method right after calling Cancel(). But I think it
breaks the cancellation pattern to be calling other functions in response to it.

[willchan no longer on Chromium, 2010/12/22 02:04:01]

I share your concerns here.  I did it just because it made the tests easy.  I
will look into reworking that.

+
+  void RunJobOnWorkerPool();
+  void CompleteJobOnOriginLoop();
+
+  // Returns true after StartJob() has been called.  Only valid to be called on
+  // the origin thread.
+  bool started() const;
+
+  // Returns true after job completes successfully or has been canceled.
+  bool done() const;
+
+  ThreadChecker thread_checker_;
+  const scoped_refptr<MessageLoopProxy> origin_loop_;
+  const bool is_slow_;
+
+  mutable Lock lock_;  // Protects all member variables below.
+  bool canceled_;
+  State state_;
+
+  DISALLOW_COPY_AND_ASSIGN(WorkerPoolJob);
+};
+
+}  // namespace base
+
+#endif  // BASE_WORKER_POOL_JOB_H_