Implementation of the database safe worker model.

Implementation of the database safe worker model.

BUG=none
TEST=unit test

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

[tim (not reviewing), 2010-01-26 01:28:48]

http://codereview.chromium.org/546138/diff/1002/1003
File chrome/browser/sync/glue/database_model_worker.cc (right):

http://codereview.chromium.org/546138/diff/1002/1003#newcode18
chrome/browser/sync/glue/database_model_worker.cc:18:
ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, NewRunnableFunction(
I'd check the return value here and DCHECK if it is false, and also run the work
synchronously to avoid deadlock.

http://codereview.chromium.org/546138/diff/1002/1003#newcode25
chrome/browser/sync/glue/database_model_worker.cc:25: base::WaitableEvent* done)
{
i'd rather not make this static to avoid the 'invoked after deletion' crash
scenario. we shouldn't get or run any such tasks after the DMW has been
destroyed. We should never end up with pending_work_ != NULL in the dtor,
because |this| should only get deleted after the syncapi has shutdown, and that
can't happen without all the MSW dispatching having terminated.  I agree with
you (from IM) that using a custom task like UIModelWorker is heavyweight.  My
vote is to make ModelSafeWorker RefCountedThreadSafe in model_safe_worker.h, use
NewRunnableMethod here, and change the calls to 'delete' in SyncBackendHost to
be Release.  WDYT?

[Paweł Hajdan Jr., 2010-01-26 06:38:07]

Drive-by with some test-related comments.

http://codereview.chromium.org/546138/diff/1002/1005
File chrome/browser/sync/glue/database_model_worker_unittest.cc (right):

http://codereview.chromium.org/546138/diff/1002/1005#newcode17
chrome/browser/sync/glue/database_model_worker_unittest.cc:17:
db_thread_.reset(new ChromeThread(ChromeThread::DB));
Why this needs to be allocated dynamically? In other tests it's stack-allocated
and it's fine. Of course SetUp is the right place for Starting the thread.

http://codereview.chromium.org/546138/diff/1002/1005#newcode36
chrome/browser/sync/glue/database_model_worker_unittest.cc:36:
ASSERT_TRUE(done.TimedWait(base::TimeDelta::FromSeconds(1)));
This seems suspiciously flaky-like. What if the bot has a slow day and takes
more than a second for a simple operation?

[albertb, 2010-01-26 22:32:38]

http://codereview.chromium.org/546138/diff/1002/1003
File chrome/browser/sync/glue/database_model_worker.cc (right):

http://codereview.chromium.org/546138/diff/1002/1003#newcode18
chrome/browser/sync/glue/database_model_worker.cc:18:
ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, NewRunnableFunction(
On 2010/01/26 01:28:48, timsteele wrote:
> I'd check the return value here and DCHECK if it is false, and also run the
work
> synchronously to avoid deadlock.
I added a DCHECK but I'm not sure what you meant by "also run the work
synchronously to avoid deadlock". Are you suggesting we don't wait for the work
to complete on the db thread before continuing?

http://codereview.chromium.org/546138/diff/1002/1003#newcode25
chrome/browser/sync/glue/database_model_worker.cc:25: base::WaitableEvent* done)
{
On 2010/01/26 01:28:48, timsteele wrote:
> i'd rather not make this static to avoid the 'invoked after deletion' crash
> scenario. we shouldn't get or run any such tasks after the DMW has been
> destroyed. We should never end up with pending_work_ != NULL in the dtor,
> because |this| should only get deleted after the syncapi has shutdown, and
that
> can't happen without all the MSW dispatching having terminated.  I agree with
> you (from IM) that using a custom task like UIModelWorker is heavyweight.  My
> vote is to make ModelSafeWorker RefCountedThreadSafe in model_safe_worker.h,
use
> NewRunnableMethod here, and change the calls to 'delete' in SyncBackendHost to
> be Release.  WDYT?

Done.

http://codereview.chromium.org/546138/diff/1002/1005
File chrome/browser/sync/glue/database_model_worker_unittest.cc (right):

http://codereview.chromium.org/546138/diff/1002/1005#newcode17
chrome/browser/sync/glue/database_model_worker_unittest.cc:17:
db_thread_.reset(new ChromeThread(ChromeThread::DB));
On 2010/01/26 06:38:07, Paweł Hajdan Jr. wrote:
> Why this needs to be allocated dynamically? In other tests it's
stack-allocated
> and it's fine. Of course SetUp is the right place for Starting the thread.

Done.

http://codereview.chromium.org/546138/diff/1002/1005#newcode36
chrome/browser/sync/glue/database_model_worker_unittest.cc:36:
ASSERT_TRUE(done.TimedWait(base::TimeDelta::FromSeconds(1)));
On 2010/01/26 06:38:07, Paweł Hajdan Jr. wrote:
> This seems suspiciously flaky-like. What if the bot has a slow day and takes
> more than a second for a simple operation?
Increased to 10 seconds.

[tim (not reviewing), 2010-01-26 22:55:14]

http://codereview.chromium.org/546138/diff/1002/1003
File chrome/browser/sync/glue/database_model_worker.cc (right):

http://codereview.chromium.org/546138/diff/1002/1003#newcode18
chrome/browser/sync/glue/database_model_worker.cc:18:
ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, NewRunnableFunction(
We could do that.. I was suggesting we do work->Run() as line 14 does.

[albertb, 2010-01-26 23:02:07]

http://codereview.chromium.org/546138/diff/1002/1003
File chrome/browser/sync/glue/database_model_worker.cc (right):

http://codereview.chromium.org/546138/diff/1002/1003#newcode18
chrome/browser/sync/glue/database_model_worker.cc:18:
ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, NewRunnableFunction(
On 2010/01/26 22:55:14, timsteele wrote:
> We could do that.. I was suggesting we do work->Run() as line 14 does. 
> 
But then it wouldn't even run on the right thread. But maybe I'm
misunderstanding the purpose of the DB thread. Is it okay (and simply not ideal)
to do DB work on any thread?

[albertb, 2010-01-26 23:20:30]

http://codereview.chromium.org/546138/diff/1002/1003
File chrome/browser/sync/glue/database_model_worker.cc (right):

http://codereview.chromium.org/546138/diff/1002/1003#newcode18
chrome/browser/sync/glue/database_model_worker.cc:18:
ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, NewRunnableFunction(
On 2010/01/26 23:02:07, albertb wrote:
> On 2010/01/26 22:55:14, timsteele wrote:
> > We could do that.. I was suggesting we do work->Run() as line 14 does. 
> > 
> But then it wouldn't even run on the right thread. But maybe I'm
> misunderstanding the purpose of the DB thread. Is it okay (and simply not
ideal)
> to do DB work on any thread?
> 
Based on discussion over chat, changed to NOTREACHED().

[tim (not reviewing), 2010-01-26 23:46:51]

http://codereview.chromium.org/546138/diff/2009/1014
File chrome/browser/sync/glue/database_model_worker_unittest.cc (right):

http://codereview.chromium.org/546138/diff/2009/1014#newcode36
chrome/browser/sync/glue/database_model_worker_unittest.cc:36:
NewCallback(&done, &base::WaitableEvent::Signal));
the callback will leak, as the chrome implementation requires the caller to own
them.

http://codereview.chromium.org/546138/diff/2009/1014#newcode37
chrome/browser/sync/glue/database_model_worker_unittest.cc:37:
ASSERT_TRUE(done.TimedWait(base::TimeDelta::FromSeconds(10)));
It's kind of confusing to have |done| here (first since the notion of "done" is
overloaded) and you don't really need it.  What I would recommend is add a
DoWork() method on the test class that does
EXPECT_TRUE(ChromeThread::CurrentlyOn(DB)), and here call
DoWorkAndWaitUntilDone(NewCallback(this, &DatabaseModelWorkerTest::DoWork)).

I think that's closer to what the DMW does in real life.

Although your impl is nice because it always terminates. I think it might be
easier to follow if you add a
OneShotTimer member to the test and just before calling DoWorkAndWaitUntilDone
tell it to Start() with a delay of 10 seconds and to invoke FAIL(), and Reset
the timer from DoWork.

[albertb, 2010-01-27 21:56:14]

Updated, I also fixed the sync_unit_tests to use scoped_refptr when holding
ModelSafeWorkers.

http://codereview.chromium.org/546138/diff/2009/1014
File chrome/browser/sync/glue/database_model_worker_unittest.cc (right):

http://codereview.chromium.org/546138/diff/2009/1014#newcode37
chrome/browser/sync/glue/database_model_worker_unittest.cc:37:
ASSERT_TRUE(done.TimedWait(base::TimeDelta::FromSeconds(10)));
On 2010/01/26 23:46:51, timsteele wrote:
> It's kind of confusing to have |done| here (first since the notion of "done"
is
> overloaded) and you don't really need it.  What I would recommend is add a
> DoWork() method on the test class that does
> EXPECT_TRUE(ChromeThread::CurrentlyOn(DB)), and here call
> DoWorkAndWaitUntilDone(NewCallback(this, &DatabaseModelWorkerTest::DoWork)).
> 
> I think that's closer to what the DMW does in real life.
> 
> Although your impl is nice because it always terminates. I think it might be
> easier to follow if you add a
> OneShotTimer member to the test and just before calling DoWorkAndWaitUntilDone
> tell it to Start() with a delay of 10 seconds and to invoke FAIL(), and Reset
> the timer from DoWork.

I've modified the test to run a OneShotTimer on the IO thread while the work is
done on the DB thread. I think it's actually harder to follow but at least it's
a closer simulation of what will really happen. :)

[tim (not reviewing), 2010-01-27 22:06:50]

yeah, you're right :) thanks for doing this though, patch looks nice.

LGTM if you already tried my suggestion and it didn't work, otherwise i'll take
a quick peek if you make the change.

http://codereview.chromium.org/546138/diff/4003/8006
File chrome/browser/sync/glue/database_model_worker_unittest.cc (right):

http://codereview.chromium.org/546138/diff/4003/8006#newcode65
chrome/browser/sync/glue/database_model_worker_unittest.cc:65: // doesn't
understand refcounting.
oh, did you try ScopedRunnableMethodFactory? it might work!

[tim (not reviewing), 2010-01-27 22:07:33]

Hmm, except for strange linux/mac errors:

[----------] 1 test from DatabaseModelWorkerTest
[ RUN      ] DatabaseModelWorkerTest.DoesWorkOnDatabaseThread
terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_S_construct NULL not valid

[albertb, 2010-01-27 23:53:56]

Hmm, I *think* that was a race between the creation of the IO ChromeThread and
the IO MessageLoop. I changed the order of the members and haven't been able to
reproduce. But I'll wait for the trybot results before submitting.

http://codereview.chromium.org/546138/diff/4003/8006
File chrome/browser/sync/glue/database_model_worker_unittest.cc (right):

http://codereview.chromium.org/546138/diff/4003/8006#newcode65
chrome/browser/sync/glue/database_model_worker_unittest.cc:65: // doesn't
understand refcounting.
On 2010/01/27 22:06:53, timsteele wrote:
> oh, did you try ScopedRunnableMethodFactory? it might work!

Changed to a ScoppedRunnableMethodFactory, that works, thanks.

[tim (not reviewing), 2010-01-28 00:02:06]

If the try succeeds (or fails in some unrelated/flaky way), this LGTM.

http://codereview.chromium.org/546138/diff/5008/5014
File chrome/browser/sync/glue/database_model_worker_unittest.cc (right):

http://codereview.chromium.org/546138/diff/5008/5014#newcode81
chrome/browser/sync/glue/database_model_worker_unittest.cc:81:
MessageLoop::current()->Run();
one final thing - may want to add setting of a bool to DoWork() (like
did_do_work_ = true) and here do EXPECT_TRUE to make sure the work was actually
done.