Use chrome/base synchronization primitives and threads instead of...

Use chrome/base synchronization primitives and threads instead of
pthreads in SyncerThread.  The old pthread impl can be used by
specifying --syncer-thread-pthreads for comparison until we
settle fully on the final impl (I have a MessageLoop-based impl
in progress).

The default SyncerThread is as close to the pthreads-impl semantics as
I could get, with one exception: it does not offer a time-out
when calling Stop(), because it greatly simplifies the implementation.
I first implemented it *with* the timeout, and for sake of
experimentation while this is in shuffle I am checking it in
as SyncerThreadTimedStop, available by using --syncer-thread-timed-stop. I'm not sure which we want ultimately, but it's useful to have around when building the MessageLoop based impl.

I had to refactor the interface slightly to allow multiple implementations,
I think it will be quite useful while working on the MessageLoop impl.

Added several tests to SyncerThreadUnittest, which all impls now pass (
just pass the command line flag to try each out).

TEST=SyncerThreadTest, SyncerThreadWithSyncerTest, integration tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27117

[tim (not reviewing), 2009-09-21 05:42:47]

The critical part of this change is the part with the least diffs, in
syncer_thread.cc, specifically ::ThreadMainLoop, ::ThreadMain, ::Start, and
::Stop.  The rest is mostly supplemental implementations for
comparison/experimentation, and preparation for a new MessageLoop based impl of
SyncerThread that will be much simpler (no CVs really needed in a post-task
world, NudgeQueue is implicitly handled by PostDelayedTask, less contention,
etc..).

And some changes for testing.

[jar (doing other things), 2009-09-22 01:12:32]

IMO, I'm a poor choice for reviewing this code, as I have no familiarity with
it, and it is rather large.

I tried to make a cursory review, focusing on the use of condition variables in
reasonable ways, and making the assumption that the existing code was correct
(I'm hopeful it was heavily reviewed previously).  It would be very helpful to
have a second reviewer that was familiar with this code.

Please consider restructuring the code to avoid using locks to block threads,
and instead consistently use locks to protect specific resources.  Condition
variables work best when used in this mode, and it will tend to prevent errors
and deadlocks.

Details below.

In a couple of the cases, items may be significant bugs.  Please also consider
adding tests that would have caught the bugs.

Thanks,

Jim

http://codereview.chromium.org/214033/diff/1021/1027
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/214033/diff/1021/1027#newcode877
Line 877: scoped_refptr<SyncerThread> syncer_thread_;
It is interesting that you used a refptr.... which suggests this may live beyond
the SyncManager.  Who holds the other refs??  Are they all on this thread??

http://codereview.chromium.org/214033/diff/1021/1031
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/214033/diff/1021/1031#newcode143
Line 143: syncer_(NULL), syncer_events_(NULL),
thread_("SyncEngine_SyncerThread"),
style nit: one initializer per line (when you can't fit it all on one line).

http://codereview.chromium.org/214033/diff/1021/1031#newcode163
Line 163: : dirman_(mgr), scm_(connection_manager),
style nit: one initializer per line

http://codereview.chromium.org/214033/diff/1021/1031#newcode224
Line 224: // loop before returning, so the caller can't call Stop before we're
I'm not sure... but I think you meant to say:

"Wait for notification that our task has run..."

There is no question that you made it onto the message loop, as the PostTask
would only return after succeeding.

http://codereview.chromium.org/214033/diff/1021/1031#newcode242
Line 242: return true;
I'm surprised you don't set a variable to preclude starting after you stopped...
or do you actually do this routinely?

I'd expect that despite the lock, you could easily race and stop first :-/. 
What thread runs these stops and starts??

http://codereview.chromium.org/214033/diff/1021/1031#newcode256
Line 256: control_field_changed_.Broadcast();
It is often better to release the lock before doing a Broadcast, as otherwise
there is often a lot of expensive thread thrashing.  (Note: You don't need to
hold a lock to signal or broadcast).

http://codereview.chromium.org/214033/diff/1021/1031#newcode287
Line 287: lock_.AssertAcquired();
In general, it is usually bad to hold locks to protect code (and it sure seems
like that is the intent).  It is generally better to use locks to protect access
to data.  I don't have a clear understanding of what data is being protected
here.

http://codereview.chromium.org/214033/diff/1021/1031#newcode294
Line 294: TimeTicks last_sync_time;
Need initializer (leads to confusion at line 318 below).

http://codereview.chromium.org/214033/diff/1021/1031#newcode363
Line 363: // TODO(timsteele): Should use Time(Delta).
It would be good to handle this.

http://codereview.chromium.org/214033/diff/1021/1031#newcode366
Line 366: int last_poll_wait,  // in s
nit: avoid abreviations

"Time in seconds"

http://codereview.chromium.org/214033/diff/1021/1031#newcode421
Line 421: thread_main_started_.Signal();
I don't see the status that changed that would cause you to do the signal.  I'm
also not clear why you wanted to do the signal while holding the lock.  I think
this is an example of using locks to block code, rather than protect resources.

http://codereview.chromium.org/214033/diff/1021/1031#newcode428
Line 428: AutoUnlock unlock(lock_);
Again, it seems like code is blocked on a lock.
Much better is to block and release on condition variables, and only briefly
acquire release a lock as you access that underlying condition.

http://codereview.chromium.org/214033/diff/1021/1031#newcode446
Line 446: TimeTicks::Now() >= nudge_queue_.top().first) {
It would appear you dropped the negation of the second line.

http://codereview.chromium.org/214033/diff/1021/1031#newcode540
Line 540: AutoLock lock(lock_);
Why do you acquire a lock before calling CheckConnected() here, but in line 533,
you don't?

http://codereview.chromium.org/214033/diff/1021/1032
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/214033/diff/1021/1032#newcode49
Line 49: static SyncerThread* Create(ClientCommandChannel* command_channel,
nit: indent static only one more than public.

http://codereview.chromium.org/214033/diff/1021/1032#newcode53
Line 53: private:
nit: indent private only one space... and fix next line as well.

http://codereview.chromium.org/214033/diff/1021/1032#newcode117
Line 117: virtual void SetSyncerPollingInterval(int interval) {
Generally for time intervals, it is nice to use base::TimeDelta

http://codereview.chromium.org/214033/diff/1021/1032#newcode120
Line 120: virtual void SetSyncerShortPollIntervalSeconds(int64 seconds) {
Suggest using TimeDelta

http://codereview.chromium.org/214033/diff/1021/1032#newcode131
Line 131: // starts the thread and used to set the value of thread_running_ to
true.
typo (or at least I'm having trouble reading):  "...starts the thread and used
to set the value..."

http://codereview.chromium.org/214033/diff/1021/1032#newcode140
Line 140: Lock lock_;
Please add a comment indicating what this lock protects.  You might also use a
longer variable name that hints about such... but I'd have to see what it
protects.

http://codereview.chromium.org/214033/diff/1021/1032#newcode148
Line 148: kNudgeRateLimitTime = 180,
I couldn't tell what units were used here, and perhaps for a time interval
TimeDelta would be clearer.

http://codereview.chromium.org/214033/diff/1021/1032#newcode161
Line 161: typedef std::priority_queue<NudgeObject, std::vector<NudgeObject>,
This is remarkably similar to the priority_queue used for posting delayed tasks
for message loops.  Any chance it should use the existing code?

http://codereview.chromium.org/214033/diff/1021/1029
File chrome/browser/sync/engine/syncer_thread_pthreads.cc (right):

http://codereview.chromium.org/214033/diff/1021/1029#newcode108
Line 108: 
nit: probably accidental insertion of newline.

http://codereview.chromium.org/214033/diff/1021/1033
File chrome/browser/sync/engine/syncer_thread_timed_stop.cc (right):

http://codereview.chromium.org/214033/diff/1021/1033#newcode41
Line 41: model_safe_worker), in_thread_main_loop_(false) {
style nit: start each constructor initializer on a new line.

http://codereview.chromium.org/214033/diff/1021/1033#newcode99
Line 99: // and unblock it's caller.
I've seen the same commentary in several files now.  In addition to noting that
locks shouldn't be used to block threads this way IMO, it would be nicer if
these descriptions were perhaps in a common header.

As noted in anther file, I think the use of "onto the message loop" in this
comment may not be correct.

[chron_chromium.org, 2009-09-22 01:13:16]

http://codereview.chromium.org/214033/diff/1021/1031
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/214033/diff/1021/1031#newcode244
Line 244: LOG(INFO) << "Setting stop_syncer_thread_ to true.";
Maybe make this log/comment clear on what stop_syncer_thread_ is intended to do.
ThreadMainBailout comment?

http://codereview.chromium.org/214033/diff/1021/1031#newcode248
Line 248: syncer_->RequestEarlyExit();
If we could, clarify the difference in a comment between RequestEarlyExit and
stop_syncer_thread_. Looks like one cops out of thread main, the other lobs out
of the syncer step itself.

http://codereview.chromium.org/214033/diff/1021/1032
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/214033/diff/1021/1032#newcode90
Line 90: virtual bool Stop(int max_wait);
It really seems like we shouldn't have any wait time at all when shutting this
thread down. I definitely support the simpler version with that in mind. We
should probably just kill any currently running network requests and just dump
the syncer overboard.

http://codereview.chromium.org/214033/diff/1021/1032#newcode139
Line 139: ConditionVariable control_field_changed_;
Maybe member_variable_changed_ ? Or as you mentioned over IM, you could contain
them in a struct.

http://codereview.chromium.org/214033/diff/1021/1026
File chrome/browser/sync/engine/syncer_thread_pthreads.h (right):

http://codereview.chromium.org/214033/diff/1021/1026#newcode46
Line 46: class SyncerThreadPthreadImpl {
Class comment please.

http://codereview.chromium.org/214033/diff/1021/1026#newcode62
Line 62: virtual bool NudgeSyncer(int milliseconds_from_now,
SyncerThread::NudgeSource source);
linewrap

http://codereview.chromium.org/214033/diff/1021/1026#newcode215
Line 215: class SyncerThreadPthreads : public SyncerThread {
Out of curiosity, why was thread config all runtime rather than compile time
kind of option?

http://codereview.chromium.org/214033/diff/1021/1026#newcode215
Line 215: class SyncerThreadPthreads : public SyncerThread {
Class comment please.

http://codereview.chromium.org/214033/diff/1021/1033
File chrome/browser/sync/engine/syncer_thread_timed_stop.cc (right):

http://codereview.chromium.org/214033/diff/1021/1033#newcode55
Line 55: LOG(INFO) << "Setting stop_syncer_thread_ to true.";
same comment as in first implementation

http://codereview.chromium.org/214033/diff/1021/1033#newcode72
Line 72: while (in_thread_main_loop_) {
This logic is a little tricky. Mention what previous call will result
in_thread_main_loop to change. (stop_syncer_thread_?)

Would it make any sense to combine in_thread_main_loop and stop_syncer_thread
into a single threadstate enum? (Haven't thought about it carefully) i.e. MAIN,
STOPPING

http://codereview.chromium.org/214033/diff/1021/1033#newcode104
Line 104: in_thread_main_loop_ = true;
Would this make sense as a threadstate enum? We could possibly then assert
certain things about thread states when we transition between them.

http://codereview.chromium.org/214033/diff/1021/1028
File chrome/browser/sync/engine/syncer_thread_timed_stop.h (right):

http://codereview.chromium.org/214033/diff/1021/1028#newcode31
Line 31: // Stop processing. A max wait of at least 2*server RTT time is
recommended.
Note here that this is implemented as opposed to our original one.

http://codereview.chromium.org/214033/diff/1021/1022
File chrome/test/sync/engine/mock_server_connection.cc (right):

http://codereview.chromium.org/214033/diff/1021/1022#newcode122
Line 122: if (mid_commit_observer_)
add squiggles my good sir

http://codereview.chromium.org/214033/diff/1021/1024
File chrome/test/sync/engine/test_directory_setter_upper.cc (right):

http://codereview.chromium.org/214033/diff/1021/1024#newcode80
Line 80: if (was_opened_)
Braces

[tim (not reviewing), 2009-09-22 03:08:43]

Thank you for detailed reviews (I had only made chron@ the reviewer and cc'ed
jar, apologies for not being clear about this in the email, I hope your review
didn't soak up too much of your time!)

Responding to a few comments, I did not make code changes yet.

http://codereview.chromium.org/214033/diff/1021/1027
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/214033/diff/1021/1027#newcode877
Line 877: scoped_refptr<SyncerThread> syncer_thread_;
On 2009/09/22 01:12:32, jar wrote:
> It is interesting that you used a refptr.... which suggests this may live
beyond
> the SyncManager.  Who holds the other refs??  Are they all on this thread??

This was a consequence of using RefCountedThreadSafe<SyncerThread> which was a
consequence of using PostTask to kick-off the main loop.

http://codereview.chromium.org/214033/diff/1021/1031
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/214033/diff/1021/1031#newcode224
Line 224: // loop before returning, so the caller can't call Stop before we're
On 2009/09/22 01:12:32, jar wrote:
> I'm not sure... but I think you meant to say:
> 
> "Wait for notification that our task has run..."
> 
> There is no question that you made it onto the message loop, as the PostTask
> would only return after succeeding.
> 
> 

It isn't about whether it made it onto the message loop, it is about whether it
has started running --this is to match the pthread implementation as close as
possible-- pthread_create did this in one shot.  If Start() returns before
ThreadMain is running this is a) different from the old impl, and b) different
from what Stop expects.

http://codereview.chromium.org/214033/diff/1021/1031#newcode242
Line 242: return true;
On 2009/09/22 01:12:32, jar wrote:
> I'm surprised you don't set a variable to preclude starting after you
stopped...
> or do you actually do this routinely?
> 
> I'd expect that despite the lock, you could easily race and stop first :-/. 
> What thread runs these stops and starts??

Start and Stop are called from the same thread. So they occur in series, one way
or another.

http://codereview.chromium.org/214033/diff/1021/1031#newcode256
Line 256: control_field_changed_.Broadcast();
On 2009/09/22 01:12:32, jar wrote:
> It is often better to release the lock before doing a Broadcast, as otherwise
> there is often a lot of expensive thread thrashing.  (Note: You don't need to
> hold a lock to signal or broadcast).

I'm aware that you don't need to hold a lock. For now I want to remain
consistent with the previous impl.  In fact, I'm almost certain in practice
there can only be one thread waiting on this so Signal is sufficient, but I'd
strongly prefer to wait until we get this first pass working and checked in
before optimizing anything.

http://codereview.chromium.org/214033/diff/1021/1031#newcode287
Line 287: lock_.AssertAcquired();
On 2009/09/22 01:12:32, jar wrote:
> In general, it is usually bad to hold locks to protect code (and it sure seems
> like that is the intent).  It is generally better to use locks to protect
access
> to data.  I don't have a clear understanding of what data is being protected
> here.

Well, you are right, it is the intent.  We plan to later implement this using
MessageLoops and posting tasks when another thread wants to notify this one that
something changed, again I prefer to keep this consistent with the previous impl
that has been in use/testing for a considerable amount of time.

http://codereview.chromium.org/214033/diff/1021/1031#newcode294
Line 294: TimeTicks last_sync_time;
On 2009/09/22 01:12:32, jar wrote:
> Need initializer (leads to confusion at line 318 below).

Hmm. what do I initialize it to if I want 0/null, which is what this does? I
hope that's not a dumb question...

http://codereview.chromium.org/214033/diff/1021/1031#newcode363
Line 363: // TODO(timsteele): Should use Time(Delta).
On 2009/09/22 01:12:32, jar wrote:
> It would be good to handle this.

Yeah... I added the TODO because I'd like to handle this in a follow up; I feel
like this change is already big enough :\

http://codereview.chromium.org/214033/diff/1021/1031#newcode421
Line 421: thread_main_started_.Signal();
On 2009/09/22 01:12:32, jar wrote:
> I don't see the status that changed that would cause you to do the signal. 
I'm
> also not clear why you wanted to do the signal while holding the lock.  I
think
> this is an example of using locks to block code, rather than protect
resources.

Signaling while holding the lock is an artifact of me first implementing
SyncerThreadTimedStop, which then sets in_thread_main_loop_ to true, and the two
need to happen together for the timed-shutdown to go properly (and so that you
can't call Stop before the thread "starts", aka ThreadMain is running). 

This was what led me to adding the StartStop test to SyncerThreadUnittest, which
will fail without this.

Apology for repeating myself but this is the semantic provided by the incumbent
implementation.  However, I will try to clean this up so it doesn't affect this
class.

http://codereview.chromium.org/214033/diff/1021/1031#newcode428
Line 428: AutoUnlock unlock(lock_);
On 2009/09/22 01:12:32, jar wrote:
> Again, it seems like code is blocked on a lock.
> Much better is to block and release on condition variables, and only briefly
> acquire release a lock as you access that underlying condition.

Perhaps, but again I'm playing a consistency trump card for now.

http://codereview.chromium.org/214033/diff/1021/1031#newcode446
Line 446: TimeTicks::Now() >= nudge_queue_.top().first) {
On 2009/09/22 01:12:32, jar wrote:
> It would appear you dropped the negation of the second line.

I flipped the inequality.

http://codereview.chromium.org/214033/diff/1021/1031#newcode540
Line 540: AutoLock lock(lock_);
On 2009/09/22 01:12:32, jar wrote:
> Why do you acquire a lock before calling CheckConnected() here, but in line
533,
> you don't?

I suspect this is because WatchConnectionManager is called before the thread has
started, so it is safe.

http://codereview.chromium.org/214033/diff/1021/1032
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/214033/diff/1021/1032#newcode90
Line 90: virtual bool Stop(int max_wait);
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> It really seems like we shouldn't have any wait time at all when shutting this
> thread down. I definitely support the simpler version with that in mind. We
> should probably just kill any currently running network requests and just dump
> the syncer overboard.

I agree, that's why I created the simpler version. But we don't have a way yet
to "kill" the current network request because the SyncerThread blocks on the
HttpBridge, which is how this code has operated since before I was working on
it.  We could/should make the net reqs asynchronous (not in this cl!)

http://codereview.chromium.org/214033/diff/1021/1032#newcode117
Line 117: virtual void SetSyncerPollingInterval(int interval) {
On 2009/09/22 01:12:32, jar wrote:
> Generally for time intervals, it is nice to use base::TimeDelta

I know it seems petty to argue about this but I really think this CL is big
enough, I'll copy/paste my TODO about using TimeDelta around to more places to
handle in a follow up.

http://codereview.chromium.org/214033/diff/1021/1032#newcode161
Line 161: typedef std::priority_queue<NudgeObject, std::vector<NudgeObject>,
On 2009/09/22 01:12:32, jar wrote:
> This is remarkably similar to the priority_queue used for posting delayed
tasks
> for message loops.  Any chance it should use the existing code?

As I mention in the change description we plan to build a MessageLoop based
SyncerThread and NudgeQueue would be implicitly handled by PostDelayedTask.  I
am not doing that in this CL, though.  I am shooting for consistency with the
previous implementation.

[tim (not reviewing), 2009-09-22 22:48:02]

http://codereview.chromium.org/214033/diff/1021/1031
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/214033/diff/1021/1031#newcode143
Line 143: syncer_(NULL), syncer_events_(NULL),
thread_("SyncEngine_SyncerThread"),
On 2009/09/22 01:12:32, jar wrote:
> style nit: one initializer per line (when you can't fit it all on one line).

Done.

http://codereview.chromium.org/214033/diff/1021/1031#newcode163
Line 163: : dirman_(mgr), scm_(connection_manager),
On 2009/09/22 01:12:32, jar wrote:
> style nit: one initializer per line

Done.

http://codereview.chromium.org/214033/diff/1021/1031#newcode244
Line 244: LOG(INFO) << "Setting stop_syncer_thread_ to true.";
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> Maybe make this log/comment clear on what stop_syncer_thread_ is intended to
do.
> ThreadMainBailout comment?

Done.

http://codereview.chromium.org/214033/diff/1021/1031#newcode248
Line 248: syncer_->RequestEarlyExit();
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> If we could, clarify the difference in a comment between RequestEarlyExit and
> stop_syncer_thread_. Looks like one cops out of thread main, the other lobs
out
> of the syncer step itself.

Done.

http://codereview.chromium.org/214033/diff/1021/1031#newcode366
Line 366: int last_poll_wait,  // in s
On 2009/09/22 01:12:32, jar wrote:
> nit: avoid abreviations
> 
> "Time in seconds"

Done.

http://codereview.chromium.org/214033/diff/1021/1032
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/214033/diff/1021/1032#newcode49
Line 49: static SyncerThread* Create(ClientCommandChannel* command_channel,
On 2009/09/22 01:12:32, jar wrote:
> nit: indent static only one more than public.

Done.

http://codereview.chromium.org/214033/diff/1021/1032#newcode53
Line 53: private:
On 2009/09/22 01:12:32, jar wrote:
> nit: indent private only one space... and fix next line as well.

Done.

http://codereview.chromium.org/214033/diff/1021/1032#newcode117
Line 117: virtual void SetSyncerPollingInterval(int interval) {
On 2009/09/22 03:08:45, timsteele wrote:
> On 2009/09/22 01:12:32, jar wrote:
> > Generally for time intervals, it is nice to use base::TimeDelta
> 
> I know it seems petty to argue about this but I really think this CL is big
> enough, I'll copy/paste my TODO about using TimeDelta around to more places to
> handle in a follow up.
> 

Well, since I added this to begin with (forgot about that) I made the change!

http://codereview.chromium.org/214033/diff/1021/1032#newcode120
Line 120: virtual void SetSyncerShortPollIntervalSeconds(int64 seconds) {
On 2009/09/22 01:12:32, jar wrote:
> Suggest using TimeDelta

Done.

http://codereview.chromium.org/214033/diff/1021/1032#newcode131
Line 131: // starts the thread and used to set the value of thread_running_ to
true.
On 2009/09/22 01:12:32, jar wrote:
> typo (or at least I'm having trouble reading):  "...starts the thread and used
> to set the value..."

Done.

http://codereview.chromium.org/214033/diff/1021/1032#newcode139
Line 139: ConditionVariable control_field_changed_;
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> Maybe member_variable_changed_ ? Or as you mentioned over IM, you could
contain
> them in a struct.

I put them in a struct.

http://codereview.chromium.org/214033/diff/1021/1032#newcode140
Line 140: Lock lock_;
On 2009/09/22 01:12:32, jar wrote:
> Please add a comment indicating what this lock protects.  You might also use a
> longer variable name that hints about such... but I'd have to see what it
> protects.

Done.

http://codereview.chromium.org/214033/diff/1021/1029
File chrome/browser/sync/engine/syncer_thread_pthreads.cc (right):

http://codereview.chromium.org/214033/diff/1021/1029#newcode108
Line 108: 
On 2009/09/22 01:12:32, jar wrote:
> nit: probably accidental insertion of newline.

Done.

http://codereview.chromium.org/214033/diff/1021/1026
File chrome/browser/sync/engine/syncer_thread_pthreads.h (right):

http://codereview.chromium.org/214033/diff/1021/1026#newcode46
Line 46: class SyncerThreadPthreadImpl {
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> Class comment please.

Done.

http://codereview.chromium.org/214033/diff/1021/1026#newcode62
Line 62: virtual bool NudgeSyncer(int milliseconds_from_now,
SyncerThread::NudgeSource source);
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> linewrap

Done.

http://codereview.chromium.org/214033/diff/1021/1026#newcode215
Line 215: class SyncerThreadPthreads : public SyncerThread {
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> Out of curiosity, why was thread config all runtime rather than compile time
> kind of option?

It's temporary, and not that useful if I have to wait for a recompile just to
test something out (e.g I'm writing a new test, seems to fail with my new impl,
hm did it work with the old one? Way easier/quicker to just pass a flag).

http://codereview.chromium.org/214033/diff/1021/1026#newcode215
Line 215: class SyncerThreadPthreads : public SyncerThread {
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> Class comment please.

Done.

http://codereview.chromium.org/214033/diff/1021/1033
File chrome/browser/sync/engine/syncer_thread_timed_stop.cc (right):

http://codereview.chromium.org/214033/diff/1021/1033#newcode41
Line 41: model_safe_worker), in_thread_main_loop_(false) {
On 2009/09/22 01:12:32, jar wrote:
> style nit: start each constructor initializer on a new line.

Done.

http://codereview.chromium.org/214033/diff/1021/1033#newcode55
Line 55: LOG(INFO) << "Setting stop_syncer_thread_ to true.";
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> same comment as in first implementation

Done.

http://codereview.chromium.org/214033/diff/1021/1033#newcode72
Line 72: while (in_thread_main_loop_) {
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> This logic is a little tricky. Mention what previous call will result
> in_thread_main_loop to change. (stop_syncer_thread_?)
> 
> Would it make any sense to combine in_thread_main_loop and stop_syncer_thread
> into a single threadstate enum? (Haven't thought about it carefully) i.e.
MAIN,
> STOPPING

Hm, maybe if they were part of the same class, but with the subclassing I think
that'll be cumbersome.

http://codereview.chromium.org/214033/diff/1021/1033#newcode99
Line 99: // and unblock it's caller.
On 2009/09/22 01:12:32, jar wrote:
> I've seen the same commentary in several files now.  In addition to noting
that
> locks shouldn't be used to block threads this way IMO, it would be nicer if
> these descriptions were perhaps in a common header.
> 
> As noted in anther file, I think the use of "onto the message loop" in this
> comment may not be correct.

Tweaked wording slightly.

http://codereview.chromium.org/214033/diff/1021/1028
File chrome/browser/sync/engine/syncer_thread_timed_stop.h (right):

http://codereview.chromium.org/214033/diff/1021/1028#newcode31
Line 31: // Stop processing. A max wait of at least 2*server RTT time is
recommended.
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> Note here that this is implemented as opposed to our original one.

Done.

http://codereview.chromium.org/214033/diff/1021/1022
File chrome/test/sync/engine/mock_server_connection.cc (right):

http://codereview.chromium.org/214033/diff/1021/1022#newcode122
Line 122: if (mid_commit_observer_)
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> add squiggles my good sir

Hm most of this file doesn't use squiggles for one-liners (if and for), so I
stuck with consistency as per style guide.  But this method seems to use
squiggles. Meh, added.

http://codereview.chromium.org/214033/diff/1021/1024
File chrome/test/sync/engine/test_directory_setter_upper.cc (right):

http://codereview.chromium.org/214033/diff/1021/1024#newcode80
Line 80: if (was_opened_)
On 2009/09/22 01:13:16, chron_chromium.org wrote:
> Braces

Done.

[chron_chromium.org, 2009-09-23 19:26:53]

http://codereview.chromium.org/214033/diff/4001/4011
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/214033/diff/4001/4011#newcode318
Line 318: // The Wait()s in these control field conditionals are not
TimedWait()s (as
probably should say vault

http://codereview.chromium.org/214033/diff/4001/4012
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/214033/diff/4001/4012#newcode47
Line 47: class SyncerThreadFactory {
Can you add a small comment on the expected lifetime of the thread, how to
invoke it and shut it down?

[tim (not reviewing), 2009-09-23 20:41:40]

http://codereview.chromium.org/214033/diff/4001/4011
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/214033/diff/4001/4011#newcode318
Line 318: // The Wait()s in these control field conditionals are not
TimedWait()s (as
On 2009/09/23 19:26:53, chron_chromium.org wrote:
> probably should say vault

Done.

http://codereview.chromium.org/214033/diff/4001/4012
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/214033/diff/4001/4012#newcode47
Line 47: class SyncerThreadFactory {
On 2009/09/23 19:26:53, chron_chromium.org wrote:
> Can you add a small comment on the expected lifetime of the thread, how to
> invoke it and shut it down?

Done.

[chron_chromium.org, 2009-09-23 20:44:54]

lgtm