Add a sequenced worker pool

content/common/sequenced_worker_pool.h

+// Copyright (c) 2011 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 CONTENT_COMMON_SEQUENCED_WORKER_POOL_H_
+#define CONTENT_COMMON_SEQUENCED_WORKER_POOL_H_
+#pragma once
+
+#include "base/callback.h"
+#include "base/memory/linked_ptr.h"
+#include "base/memory/ref_counted.h"
+#include "base/tracked_objects.h"
+
+// A worker thread pool that enforces ordering between sets of tasks. It also
+// allows you to specify what should happen to your tasks on shutdown.
+//
+// To enforce ordering, get a unique sequence token from the pool and post all
+// tasks you want to order with the token. All tasks with the same token are
+// guaranteed to execute serially, though not necessarily on the same thread.
+//
+// Example:
+//   SequencedWorkerPool::SequenceToken token = pool.GetSequenceToken();
+//   pool.PostSequencedWorkerTask(token, SequencedWorkerPool::SKIP_ON_SHUTDOWN,
+//                                FROM_HERE, base::Bind(...));

[jar (doing other things), 2011/10/29 02:37:14]

Although I have yet to figure out how to do it, I *wish* we didn't have to see
the "FROM_HERE" macro.  As a result, I put it first in the argument list, so
that:
a) It blends with the stuff you don't have to read.
b) If we ever figure out a way to make this better, we can search and replace
the Post...(FROM_HERE with something better. 

Bottom line: I'd humbly request FROM_HERE be listed as a first arg. This will
put less of this garbage in the middle of "real" arguments which the author has
to get right.

+//   pool.PostSequencedWorkerTask(token, SequencedWorkerPool::SKIP_ON_SHUTDOWN,
+//                                FROM_HERE, base::Bind(...));
+//
+// You can also post tasks to the pool without ordering using PostWorkerTask.
+// These will be executed in an unspecified order. The order of execution
+// between tasks with different sequence tokens is also unspecified.
+//
+// This class is designed to be leaked on shutdown to allow the
+// CONTINUE_ON_SHUTDOWN behavior to be implemented. To enforce the
+// BLOCK_SHUTDOWN behavior, you must call Shutdown() which will wait until

[jar (doing other things), 2011/10/29 02:37:14]

I think we want most folks to not block shutdown.  As a result, we should make
it mean and obvious when they are blocking (or going to block) shutdown.  For
example, perhaps we need a long name like:
SequenceWorkerPool::REALY_REALY_REALY_HANG_THE_BROWSER_UNTIL_DONE

I want a code reviewer reading this to be VERY aware that the code is insisting
on this behavior.

+// the necessary tasks have completed.
+//
+// Implementation note: This does not use a base::WorkerPool since that does
+// not enforce shutdown semantics or allow us to specify how many worker
+// threads to run. For the typical use case of random background work, we don't
+// necessarily want to be super aggressive about creating threads.
+class SequencedWorkerPool {
+ public:
+  // Defines what should happen to a task posted to the worker pool on shutdown.
+  enum WorkerShutdown {
+    // Tasks posted with this mode which have not run at shutdown will be
+    // deleted rather than run, and any tasks with this mode running at
+    // shutdown will be ignored (the worker thread will not be joined).
+    //
+    // This option provides a nice way to post stuff you don't want blocking
+    // shutdown. For example, you might be doing a slow DNS lookup and if it's
+    // blocked on the OS, you may not want to stop shutdown, since the result
+    // doesn't really matter at that point.
+    //
+    // However, you need to be very careful what you do in your callback when
+    // you use this option. Since the thread will continue to run until the OS
+    // terminates the process, the app can be in the process of tearing down
+    // when you're running. This means any singletons or global objects you
+    // use may suddenly become invalid out from under you. For this reason,
+    // it's best to use this only for slow but simple operations like the DNS
+    // example.
+    CONTINUE_ON_SHUTDOWN,
+
+    // Tasks posted with this mode that have not started executing at shutdown
+    // will be deleted rather than executed. However, tasks already in progress
+    // will be completed.
+    SKIP_ON_SHUTDOWN,

[jar (doing other things), 2011/10/29 02:37:14]

This is a confusing distinction with CONTINUE_ON_SHUTDOWN.
When you say that "tasks already in progress" will be completed, do you mean you
will join the thread? Wait for completion before teardown?

I guess you're going after something subtle, that some tasks, once started,
become shutdown blockers.  IF they don't start, then they are not blockers, eh?

[brettw, 2011/10/29 05:31:18]

Yes. SKIP_ON_SHUTDOWN is the default for the current file thread (I think we
empty the task queue and join with the tread, but don't quote me on that).

This is what I think of as the default. I'm worried about the "we'll forget
about you on shutdown" because I think it will be too easy to write crashing
code. Obviously BLOCK has other disadvantages.

+
+    // Tasks posted with this mode will block browser shutdown until they're
+    // executed. Since this can have significant performance implications, use
+    // sparingly.

[jar (doing other things), 2011/10/29 02:37:14]

What happens if a BLOCK_SHUTDOWN task appears in line (with a given token) after
less aggressive  (SKIP_ON_SHUTDOWN, etc.) tasks with the same token?  Do you
sacrifice the ordering rule (discard the less important tasks, and run the
shutdown blocker first), or do you sacrifice the meaning of CONTINUE_ON_SHUTDOWN
and SKIP_ON_SHUTDOWN, and make the browser wait for all of them?

IMO, this suggests that a given toke should be tagged with a completion enum,
rather than an individual task.

What am I missing? 

[brettw, 2011/10/29 05:31:18]

Interesting point, I missed that. I'll move the shutdown mode to the token for
the sequenced case.

+    //
+    // Generally, this should be used only for user data, for example, a task
+    // writing a preference file.
+    //
+    // If a task is posted during shutdown, it will not get run since the
+    // workers may already be stopped. In this case, the post operation will
+    // fail (return false) and the task will be deleted.

[jar (doing other things), 2011/10/29 02:37:14]

Hmm.. that sounds like a confusing or racy definition.  If you get into a queue,
then you get to run... 

This might work... but I'm a little skeptical about racing.

[brettw, 2011/10/29 05:31:18]

I didn't know how else to define this. I wanted a way to write files out, like
the visited link file where you post tasks to the FILE thread to write little
bits to it as stuff happens (in this example, you probably won't get updates
during shutdown, but anyway...).

I want these updates to get written if the user quits, since links will render
incorrectly for subsequent runs if they get dropped.

If I do a task during shutdown, what happens? Today I do
BrowserThread::PostTask(BrowserThread::FILE which then drops the task if we
already joined with the file thread. From reading the comments on those
functions, it looks like those functions even have a third mode where Quit is
already posted to the thread but the thread still exists, in which case the task
is dropped but PostTask still returns true.

So with this model, at least you know what happens (the return values tells you
whether it's guaranteed to run or not). The only other option is to just run the
task right there before returning which seems like a bad idea to me.

+    BLOCK_SHUTDOWN,
+  };
+
+  // Opaque identifier that defines sequencing of tasks posted to the worker
+  // pool. See NewSequenceToken().
+  class SequenceToken {
+   public:
+    SequenceToken() : id_(0) {}
+    ~SequenceToken() {}
+
+   private:
+    friend SequencedWorkerPool;
+
+    SequenceToken(int id) : id_(id) {}
+
+    int id_;

[jar (doing other things), 2011/10/29 02:37:14]

This probably needs to be an atomic, or more likely, you need a lock to protect
access when incrementing.

[brettw, 2011/10/29 05:31:18]

This isn't changed, it's the value returned to the caller that never changes (I
should probably make this const). The thing that's incremented is in the Inner
class.

+  };
+
+  // Allows tests to perform certain actions.
+  class TestingObserver {
+   public:
+    virtual void WillWaitForShutdown() = 0;
+  };
+
+  SequencedWorkerPool(size_t max_threads);
+  ~SequencedWorkerPool();
+
+  // Returns a unique token that can be used to sequence tasks posted to
+  // PostSequencedWorkerTask(). Valid tokens are alwys nonzero.

[jar (doing other things), 2011/10/29 02:37:14]

nit: alwys

+  SequenceToken GetSequenceToken();
+
+  // Posts the given task for execution in the worker pool. Tasks posted with
+  // this function will execute in an unspecified order on a background thread.
+  // Returns true if the task was posted. If your tasks have ordering
+  // requirements, see PostSequencedWorkerTask().
+  //
+  // Returns true if the task was posted successfully. This may fail during
+  // shutdown regardless of the specified ShutdownBehavior.
+  bool PostWorkerTask(WorkerShutdown shutdown_behavior,
+                      const tracked_objects::Location& from_here,
+                      const base::Closure& task);
+
+  // Like PostWorkerTask above, but provides sequencing semantics. This means
+  // that tasks posted with the same sequence token (see GetSequenceToken())
+  // are guaranteed to execute in order. This is useful in cases where you're
+  // doing operations that may depend on previous ones, like appending to a
+  // file.
+  //
+  // Returns true if the task was posted successfully. This may fail during
+  // shutdown regardless of the specified ShutdownBehavior.

[jar (doing other things), 2011/10/29 02:37:14]

I guess it is good that you have weasel words.  Can we do better though?

[brettw, 2011/10/29 05:31:18]

See discussion above, I don't think we can do better.

+  bool PostSequencedWorkerTask(SequenceToken sequence_token,
+                               WorkerShutdown shutdown_behavior,
+                               const tracked_objects::Location& from_here,
+                               const base::Closure& task);
+
+  // Implements the worker pool shutdown. This should be called during app
+  // shutdown, and will discard/join with appropriate tasks before returning.
+  // After this call, subsequent calls to post tasks will fail.
+  void Shutdown();
+
+  // Called by tests to set the testing observer. This is NULL by default
+  // and ownership of the pointer is kept with the caller.
+  void SetTestingObserver(TestingObserver* observer);
+
+ private:
+  class Inner;
+  scoped_refptr<Inner> inner_;
+
+  DISALLOW_COPY_AND_ASSIGN(SequencedWorkerPool);
+};
+
+#endif  // CONTENT_COMMON_SEQUENCED_WORKER_POOL_H_