sync: hook up ServerConnectionManager <> SyncerThread2 and tie up more loose ends

sync: hook up ServerConnectionManager <> SyncerThread2 and tie up more loose ends

This permits the thread to initiate syncing by waiting for valid network connectivity.

Adds a callback to SyncerThread2::Start to learn when config mode has started.

Makes ConfigureDataTypes asynchronous to allow waiting for config mode to have started.

As a result of this, passing --new-syncer-thread on the command line actually works and doesn't crash (too much!)

BUG=26339
TEST=SyncerThread2Test

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

[tim (not reviewing), 2011-03-24 23:25:59]

Hey guys

You should both feel free to review everything, but to try and make lives
simpler:

Lingesh, can you look at syncer_thread2{h,cc} and ServerConnectionManager

Nicolas, can you look at the glue / SBH / DTM changes, since it probably ties
pretty closely to your current patch :)

[tim (not reviewing), 2011-03-24 23:28:27]

+atwilson FYI

Also note that some bugs need to be filed for future work - e.g. we currently
drop all nudges on startup due to being in Configuration mode.  The SyncerThread
should just remember if a nudge was received when in that mode, and when config
ends, determine if it still needs to run it (i.e. any unsynced local items
exist, or a tango notification came in.)

[Nicolas Zea, 2011-03-25 18:53:03]

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:2149: sync_manager_->RequestNudge();
Do we make a new connection to the server on an ip address change? If so, your
OnServerConnectionEvent in SyncerThread should be able to handle the nudge,
instead of here, right?

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.h:148: void
UpdateCarryoverSessionState(const SyncSessionJob& old_job);
Why is it important to do this?

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/update_applicator.cc:99: // be unapplied items from a
previous sync.
It's not clear to me why the conditions are valid. Any newly added type should
already be in the routing info right?

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:416: if
(pending_config_mode_state_->deleted_type) {
Should we check pending_config_mode_state_.get() first? Since below we transfer
the pending_config_mode to pending_download_state, but I'm not seeing where we
transfer it back.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:443: if (need_download) {
Presumably with the new syncer implementation we can reach here if we had an old
sync cycle finish before we entered configuration mode. A comment explaining why
this is handled correctly would be useful.

[lipalani1, 2011-03-25 23:18:30]

My initial set of feedback. Rest looks good actually. But once you have
answered/addressed this I would like to take one more look.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.cc (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:93: server_connection_ok_ = true;
This seems to be a little odd. There is no catch all else condition probably to
leave the variable server_connection_ok_ untouched in that case.

A better approach might be to use the server_reachable_ value that we get as
part of the event listener notification.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:97: void SyncerThread::Start(Mode
mode, ModeChangeCallback* callback) {
I am assuming start can be called from only one thread(sync_core?) it might
makes sense to add a DCHECK here on the thread that is calling it.
I assuming,based on the lack of locks, it is not designed to be called from
multiple threads.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:126:
CheckServerConnectionManagerStatus(scm->server_status());
The name(CheckServerConnectionManagerStatus) seems a little confusing here. This
actually sets the server_connect_ok_ right? what does it check?

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:138: callback->Run();
This callback seems to be never used? what is the purpose of this? Future use?

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:270: }
Is this needed here? When we run the job we check shouldrunjob once more and in
fact at that time we may be able to run this job...

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:400: return;
We need some kind of logging here.
I am assuming the following answers are no. But can you confirm?
1. when clear user data command is issued would we ever drop it?

[tim (not reviewing), 2011-03-27 19:23:19]

thanks for the attentive review.  will post updated patch set soon, just
addressed some Qs first.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.cc (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:93: server_connection_ok_ = true;
On 2011/03/25 23:18:30, lipalani1 wrote:
> This seems to be a little odd. There is no catch all else condition probably
to
> leave the variable server_connection_ok_ untouched in that case.
> 
> A better approach might be to use the server_reachable_ value that we get as
> part of the event listener notification.

We surface both 'reachable' and 'server up' and 'server broken' in about:sync
and they mean 3 different things.  Not sure what you're proposing? Note,
this was copy/pasted from the existing code in syncer_thread.cc and i'm trying
to keep parity where possible.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:97: void SyncerThread::Start(Mode
mode, ModeChangeCallback* callback) {
On 2011/03/25 23:18:30, lipalani1 wrote:
> I am assuming start can be called from only one thread(sync_core?) it might
> makes sense to add a DCHECK here on the thread that is calling it.
> I assuming,based on the lack of locks, it is not designed to be called from
> multiple threads.

Right.  We can't easily add a DCHECK though since the core thread is not a "well
known" browser thread.  I could build something I suppose to stash the
created-on-message loop and dcheck, but I'm not sure that's what you wanted...
One day soon I hope to eliminate core_thread altogether by exposing a
MessageLoopProxy from the syncer thread interface.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:126:
CheckServerConnectionManagerStatus(scm->server_status());
On 2011/03/25 23:18:30, lipalani1 wrote:
> The name(CheckServerConnectionManagerStatus) seems a little confusing here.
This
> actually sets the server_connect_ok_ right? what does it check?

It checks the passed-in value to figure out what to set the value to.  Agree it
could have a better name (again this was copied from syncer_thread). 
UpdateServerConnectionStatus?  That's a bit ambiguous too but maybe better.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:138: callback->Run();
On 2011/03/25 23:18:30, lipalani1 wrote:
> This callback seems to be never used? what is the purpose of this? Future use?

See StartConfigurationMode in syncapi -- we use this when the user changes the
set of active data types to alert the DTM when it's safe to proceed with
associations.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:270: }
On 2011/03/25 23:18:30, lipalani1 wrote:
> Is this needed here? When we run the job we check shouldrunjob once more and
in
> fact at that time we may be able to run this job...

I originally thought the same, but it turns out it doesn't  make sense to
construct a session when we're in CONFIGURATION mode because we're configuring
things like the model safe routing info.

As a result, we drop some nudges during config.  So this was what my commet in
the CL email was about.  We need to track if we dropped a nudge during config. 
We could either just run it, or optimize by checking if there are unsynced items
or a pending notification.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.cc:400: return;
On 2011/03/25 23:18:30, lipalani1 wrote:
> We need some kind of logging here.
> I am assuming the following answers are no. But can you confirm?
> 1. when clear user data command is issued would we ever drop it?

good question -- see bug 71616.  We currently do drop it but mainly for
consistency with other impl and simplicity for now, need to fix in future.

I can add a LOG WARNING here too.

[tim (not reviewing), 2011-03-28 20:02:25]

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:2149: sync_manager_->RequestNudge();
On 2011/03/25 18:53:03, nzea wrote:
> Do we make a new connection to the server on an ip address change? If so, your
> OnServerConnectionEvent in SyncerThread should be able to handle the nudge,
> instead of here, right?

We don't but see TODO(tim) in syncer_thread2.h that discusses reacting to a
change there.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread2.h (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread2.h:148: void
UpdateCarryoverSessionState(const SyncSessionJob& old_job);
On 2011/03/25 18:53:03, nzea wrote:
> Why is it important to do this?

added comment.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/update_applicator.cc:99: // be unapplied items from a
previous sync.
On 2011/03/25 18:53:03, nzea wrote:
> It's not clear to me why the conditions are valid. Any newly added type should
> already be in the routing info right?

Right, but types that were already enabled but not currently in the routing info
need to be skipped.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:416: if
(pending_config_mode_state_->deleted_type) {
On 2011/03/25 18:53:03, nzea wrote:
> Should we check pending_config_mode_state_.get() first? Since below we
transfer
> the pending_config_mode to pending_download_state, but I'm not seeing where we
> transfer it back.

Discussed offline, think this is fine.

http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:443: if (need_download) {
On 2011/03/25 18:53:03, nzea wrote:
> Presumably with the new syncer implementation we can reach here if we had an
old
> sync cycle finish before we entered configuration mode. A comment explaining
why
> this is handled correctly would be useful.

Discussed offline, we don't wind up here but HandleSyncCycleCompleted... which
only looks at pending_download_state.

[Nicolas Zea, 2011-03-28 20:13:51]

LGTM

[lipalani, 2011-03-28 20:59:32]

CIL

On Sun, Mar 27, 2011 at 3:23 PM, <tim@chromium.org> wrote:

> thanks for the attentive review.  will post updated patch set soon, just
> addressed some Qs first.
>
>
>
>
>
http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
> File chrome/browser/sync/engine/syncer_thread2.cc (right):
>
>
>
http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/syncer_thread2.cc:93: server_connection_ok_ =
> true;
> On 2011/03/25 23:18:30, lipalani1 wrote:
>
>> This seems to be a little odd. There is no catch all else condition
>>
> probably to
>
>> leave the variable server_connection_ok_ untouched in that case.
>>
>
>  A better approach might be to use the server_reachable_ value that we
>>
> get as
>
>> part of the event listener notification.
>>
>
> We surface both 'reachable' and 'server up' and 'server broken' in
> about:sync and they mean 3 different things.  Not sure what you're
> proposing? Note,
> this was copy/pasted from the existing code in syncer_thread.cc and i'm
> trying to keep parity where possible.

[lipalani] - I think exposing server_reachable_ variable(which we get from
the notification) is probably the correct thing(unless you can correct me!!)
That seems to be what we need.

>
>
>
>
http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/syncer_thread2.cc:97: void
> SyncerThread::Start(Mode mode, ModeChangeCallback* callback) {
> On 2011/03/25 23:18:30, lipalani1 wrote:
>
>> I am assuming start can be called from only one thread(sync_core?) it
>>
> might
>
>> makes sense to add a DCHECK here on the thread that is calling it.
>> I assuming,based on the lack of locks, it is not designed to be called
>>
> from
>
>> multiple threads.
>>
>
> Right.  We can't easily add a DCHECK though since the core thread is not
> a "well known" browser thread.  I could build something I suppose to
> stash the created-on-message loop and dcheck, but I'm not sure that's
> what you wanted... One day soon I hope to eliminate core_thread
> altogether by exposing a MessageLoopProxy from the syncer thread
> interface.

[lipalani] - Cant you just use currentthread()::messageloop and
core_thread_::messageloop? and see if they are equal?

>
>
>
>
http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/syncer_thread2.cc:126:
> CheckServerConnectionManagerStatus(scm->server_status());
> On 2011/03/25 23:18:30, lipalani1 wrote:
>
>> The name(CheckServerConnectionManagerStatus) seems a little confusing
>>
> here. This
>
>> actually sets the server_connect_ok_ right? what does it check?
>>
>
> It checks the passed-in value to figure out what to set the value to.
> Agree it could have a better name (again this was copied from
> syncer_thread).  UpdateServerConnectionStatus?  That's a bit ambiguous
> too but maybe better.

[lipalani - sounds good.

>
>
>
http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/syncer_thread2.cc:138: callback->Run();
> On 2011/03/25 23:18:30, lipalani1 wrote:
>
>> This callback seems to be never used? what is the purpose of this?
>>
> Future use?
>
> See StartConfigurationMode in syncapi -- we use this when the user
> changes the set of active data types to alert the DTM when it's safe to
> proceed with associations.

[lipalani] - sounds good.

>
>
http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/syncer_thread2.cc:270: }
> On 2011/03/25 23:18:30, lipalani1 wrote:
>
>> Is this needed here? When we run the job we check shouldrunjob once
>>
> more and in
>
>> fact at that time we may be able to run this job...
>>
>
> I originally thought the same, but it turns out it doesn't  make sense
> to construct a session when we're in CONFIGURATION mode because we're
> configuring things like the model safe routing info.
>
> As a result, we drop some nudges during config.  So this was what my
> commet in the CL email was about.  We need to track if we dropped a
> nudge during config.  We could either just run it, or optimize by
> checking if there are unsynced items or a pending notification.


[lipalani] - sounds good.

>
>
http://codereview.chromium.org/6690020/diff/12001/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/syncer_thread2.cc:400: return;
> On 2011/03/25 23:18:30, lipalani1 wrote:
>
>> We need some kind of logging here.
>> I am assuming the following answers are no. But can you confirm?
>> 1. when clear user data command is issued would we ever drop it?
>>
>
> good question -- see bug 71616.  We currently do drop it but mainly for
> consistency with other impl and simplicity for now, need to fix in
> future.
>
> I can add a LOG WARNING here too.
>
>
> [lipalani] - sounds good

> http://codereview.chromium.org/6690020/
>

[tim (not reviewing), 2011-03-28 21:58:14]

(fyi prefer replying using the tool when possible 'cause it's easier to continue
the conversation :) )

the syncer thread has no concept of the "core thread" as they are at two
completely separate layers :\

[lipalani1, 2011-03-28 23:46:21]

Cool. 
Are you planning on sending a review for the other pending feedback? 
thanks!
regards,
Lingesh
On 2011/03/28 21:58:14, timsteele wrote:
> (fyi prefer replying using the tool when possible 'cause it's easier to
continue
> the conversation :) )
> 
> the syncer thread has no concept of the "core thread" as they are at two
> completely separate layers :\

[tim (not reviewing), 2011-03-29 15:22:05]

The 'reachable' part is exposed in about:sync currently (see AllStatus for how
that's wired up).  The code in here is used to figure out if sync can't move
forward and should just wait for something to happen. E.g. if we get any of the
other codes (like IO_ERROR or SYNC_SERVER_ERROR), here, we treat them as
transient errors and don't modify the sync online/offline-ness.  Does that make
sense?  Even if we wanted to change it, I want to keep parity with the old
syncer thread for this checkin.

[lipalani1, 2011-03-29 15:26:56]

Yeah I think the parity part makes sense. 
LGTM from my side I think!
On 2011/03/29 15:22:05, timsteele wrote:
> The 'reachable' part is exposed in about:sync currently (see AllStatus for how
> that's wired up).  The code in here is used to figure out if sync can't move
> forward and should just wait for something to happen. E.g. if we get any of
the
> other codes (like IO_ERROR or SYNC_SERVER_ERROR), here, we treat them as
> transient errors and don't modify the sync online/offline-ness.  Does that
make
> sense?  Even if we wanted to change it, I want to keep parity with the old
> syncer thread for this checkin.