Server directed error handling backend code.

Server directed error handling backend code.

BUG=94007, 70276
TEST=sync_integration_tests.exe


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

[lipalani1, 2011-08-19 17:49:14]

Please review.
This is the backend code. The frontend code to handle this error in pss and
above would be another CL.
almost all files except syncer_proto_util.cc should only have few lines of code
each(mostly plumbing). the meat is in the syncer_proto_util and
sync_scheduler.cc.

[tim (not reviewing), 2011-08-22 15:14:57]

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:278: bool
ShouldRequestEarlyExit(const sync_api::SyncError& error) {
Having this method here is a layering violation in my mind.  I feel like this is
something the SyncSession::Delegate should figure out.

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:284: case sync_api::THROTTLED:
We shouldn't be requesting early exit on throttled...

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:286: case
sync_api::TRANSIENT_ERROR:
We definitely should not be requesting early exit on transient error.  In
general too, let's try to minimize the changes in behavior with the current
impl.

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:287: case
sync_api::INVALID_CREDENTIAL:
I think (as we discussed in review meetings) invalid credentials is a separate
case?  I'm not sure what the difference between this, an RC_UNAUTHORIZED server
connection manager code, or browser_sync::SYNC_AUTH_ERROR.  Two was too many to
keep track of, I'd rather not add a 3rd auth state.

[lipalani1, 2011-08-22 21:43:53]

Please review once more.

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:278: bool
ShouldRequestEarlyExit(const sync_api::SyncError& error) {
hmm... This is how it is currently implemented and I think it is right as well.
In the current impl if we get a birthday error SyncerProtoUtil figures that out
and asks the delegate to stop syncing. Same for throttled. I think that is the
correct layering as well.
On 2011/08/22 15:14:57, timsteele wrote:
> Having this method here is a layering violation in my mind.  I feel like this
is
> something the SyncSession::Delegate should figure out.

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:284: case sync_api::THROTTLED:
On 2011/08/22 15:14:57, timsteele wrote:
> We shouldn't be requesting early exit on throttled...

Done.

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:286: case
sync_api::TRANSIENT_ERROR:
Agree. it has already caused grievance for me :) With throttled and temporary
error the server might not send us any action.(there is nothing the user can
do). So the PSS would receive neither the sync_cycle_ended notification nor the
sync_error notification. if this is a first time sync we would end up with
infinite spin!
I am rewriting this. 
On 2011/08/22 15:14:57, timsteele wrote:
> We definitely should not be requesting early exit on transient error.  In
> general too, let's try to minimize the changes in behavior with the current
> impl. 
> 

Done.

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:287: case
sync_api::INVALID_CREDENTIAL:
It is a seperate case. The event for 401 error is handled by
|PostAndprocessHeader|. Ideally we should not have the invalid auth error in the
protocol but only in the http header. But just in case server sends the auth
error we need to handle it.(currently as per the client code if the server ever
sends the error in protobuf we will hit a notreached).
On 2011/08/22 15:14:57, timsteele wrote:
> I think (as we discussed in review meetings) invalid credentials is a separate
> case?  I'm not sure what the difference between this, an RC_UNAUTHORIZED
server
> connection manager code, or browser_sync::SYNC_AUTH_ERROR.  Two was too many
to
> keep track of, I'd rather not add a 3rd auth state.

[tim (not reviewing), 2011-08-23 14:17:07]

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:278: bool
ShouldRequestEarlyExit(const sync_api::SyncError& error) {
On 2011/08/22 21:43:53, lipalani1 wrote:
> hmm... This is how it is currently implemented and I think it is right as
well.
> In the current impl if we get a birthday error SyncerProtoUtil figures that
out
> and asks the delegate to stop syncing. Same for throttled. I think that is the
> correct layering as well.
> On 2011/08/22 15:14:57, timsteele wrote:
> > Having this method here is a layering violation in my mind.  I feel like
this
> is
> > something the SyncSession::Delegate should figure out. 
> 

That's because we used an existing error ("birthday error" in this case) for a
new feature "stop syncing permanently", which wasn't ideal. And moreover, "stop
syncing permanently" in that case means a user-level "stop sync" operation --
not a low level thing like stopping the loop in sync scheduler.  Nothing should
know about that except the scheduler.

And it doesn't work that way for throttled, because we don't stop syncing. We
just tell the delegate "server said not to sync again until x" and the scheduler
figures out what to do internally. 

SyncerProtoUtil should be as uncoupled as possible from any implementation
details like the fact that the scheduler has to tell the Syncer to exit a loop;
that's why the delegate interface exists.

http://codereview.chromium.org/7621085/diff/9002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_proto_util.cc:287: case
sync_api::INVALID_CREDENTIAL:
On 2011/08/22 21:43:53, lipalani1 wrote:
> It is a seperate case. The event for 401 error is handled by
> |PostAndprocessHeader|. Ideally we should not have the invalid auth error in
the
> protocol but only in the http header. But just in case server sends the auth
> error we need to handle it.(currently as per the client code if the server
ever
> sends the error in protobuf we will hit a notreached).
> On 2011/08/22 15:14:57, timsteele wrote:
> > I think (as we discussed in review meetings) invalid credentials is a
separate
> > case?  I'm not sure what the difference between this, an RC_UNAUTHORIZED
> server
> > connection manager code, or browser_sync::SYNC_AUTH_ERROR.  Two was too many
> to
> > keep track of, I'd rather not add a 3rd auth state.
> 

I agree with what you say here, but I still don't see why we are now adding
another error code if you agree we already have too many :)

[tim (not reviewing), 2011-08-23 14:18:28]

Also, please update the BUG= and TEST=?
I thought there was already a bug for this, but if not please add one.

[lipalani1, 2011-08-24 03:25:26]

Please review.

[tim (not reviewing), 2011-08-24 14:47:51]

Few more comments.  Bug 70276 is also relevant.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1120: void
SyncScheduler::OnRequestEarlyExit() {
I think we should inline this method for now, and the MessageLoop DCHECK should
go in OnSyncError instead.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1135: void
SyncScheduler::OnSyncError(sessions::SyncSession* session) {
We should just pass the error here, and / or just a session snapshot.  In
general we should prefer passing snapshots around than sessions, since we
usually never need access to the full mutable session object.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1141: }
file doesn't have braces on single line ifs

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.h:26: #include
"chrome/browser/sync/sessions/sync_error.h"
Given that the convention here is having enums and structs in this file, I
probably would have kept things here... but I won't insist.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.h:136: SyncError sync_error);
I think this belongs in SyncerStatus or ErrorCounters, probably the latter
though I agree the naming is not ideal.  but snapshot.errors and
snapshot.sync_error smells bad, as does adding yet-another-parameter to this
constructor. The structs above are intended to reduce the impact of adding
things here.

I would add it to ErrorCounters and rename that to just Errors or ErrorState
(browser_sync::sessions::ErrorState).  You can add a TODO for the latter.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_error.h (right):

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_error.h:16: SUCCESS,
each case here should have comments.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_error.h:36: struct SyncError {
Can we call this 'SyncProtocolError' to make it a bit less ambiguous?
Likewise, SyncProtocolErrorType above.

[tim (not reviewing), 2011-08-24 17:50:50]

re: my SyncError / ProtocolError comments... 
maybe the ServerConnectionManager isn't a bad place to have the definition,
either.

[lipalani1, 2011-08-25 06:14:06]

Please review.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1120: void
SyncScheduler::OnRequestEarlyExit() {
On 2011/08/24 14:47:51, timsteele wrote:
> I think we should inline this method for now, and the MessageLoop DCHECK
should
> go in OnSyncError instead.

Done.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1135: void
SyncScheduler::OnSyncError(sessions::SyncSession* session) {
The problem is we need the snapshot/error and the context(to send the
notification). Instead of splitting them out it makes to pass the session which
is meant to hold this stuff.
On 2011/08/24 14:47:51, timsteele wrote:
> We should just pass the error here, and / or just a session snapshot.  In
> general we should prefer passing snapshots around than sessions, since we
> usually never need access to the full mutable session object.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1141: }
On 2011/08/24 14:47:51, timsteele wrote:
> file doesn't have braces on single line ifs

Done.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.h:26: #include
"chrome/browser/sync/sessions/sync_error.h"
That was my first thought too. But I am adding another 100 lines,so decided to
make it a new file.
On 2011/08/24 14:47:51, timsteele wrote:
> Given that the convention here is having enums and structs in this file, I
> probably would have kept things here... but I won't insist.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.h:136: SyncError sync_error);
Ha ha went through the same argument in my head about adding to the constructor.
My idea here was to leave it seperate and add a todo to clean up this structure.
For example to take this one step further there is no reason why all of this
should not be a part of SyncerStatus(or a new syncerstatus structure that we
define). It currently has things like num_commits etc. Things like
has_more_to_sync, throttled etc should be a part of it as well as should the
error_counter. I am not fond of making the protocol error part of error_counter
even temporarily. But either way I am not passionate on this. So ok to change if
you feel there is a compelling reason. Otherwise I can leave a todo here to
clean up snapshot and file a bug. Currently it is renamed to
sync_protocol_error.(Also if we go this route, My thinking was this might be a
good starter bug for new people because it will introduce all the important
layers of sync. from syncer to ui, in a gentle(!) manner. ).
On 2011/08/24 14:47:51, timsteele wrote:
> I think this belongs in SyncerStatus or ErrorCounters, probably the latter
> though I agree the naming is not ideal.  but snapshot.errors and
> snapshot.sync_error smells bad, as does adding yet-another-parameter to this
> constructor. The structs above are intended to reduce the impact of adding
> things here.
> 
> I would add it to ErrorCounters and rename that to just Errors or ErrorState
> (browser_sync::sessions::ErrorState).  You can add a TODO for the latter.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_error.h (right):

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_error.h:16: SUCCESS,
On 2011/08/24 14:47:51, timsteele wrote:
> each case here should have comments.

Done.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_error.h:36: struct SyncError {
On 2011/08/24 14:47:51, timsteele wrote:
> Can we call this 'SyncProtocolError' to make it a bit less ambiguous?
> Likewise, SyncProtocolErrorType above.

Done.

[tim (not reviewing), 2011-08-25 15:58:54]

Please add 70276 to the BUG=.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1135: void
SyncScheduler::OnSyncError(sessions::SyncSession* session) {
On 2011/08/25 06:14:06, lipalani1 wrote:
> The problem is we need the snapshot/error and the context(to send the
> notification). Instead of splitting them out it makes to pass the session
which
> is meant to hold this stuff.
> On 2011/08/24 14:47:51, timsteele wrote:
> > We should just pass the error here, and / or just a session snapshot.  In
> > general we should prefer passing snapshots around than sessions, since we
> > usually never need access to the full mutable session object.
> 

The scheduler owns the context, so there's no need to pass it.  You can pass in
the snapshot, and the scheduler can use its context pointer.

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.h:26: #include
"chrome/browser/sync/sessions/sync_error.h"
On 2011/08/25 06:14:06, lipalani1 wrote:
> That was my first thought too. But I am adding another 100 lines,so decided to
> make it a new file.
> On 2011/08/24 14:47:51, timsteele wrote:
> > Given that the convention here is having enums and structs in this file, I
> > probably would have kept things here... but I won't insist.
> 

OK, actually a new file makes sense here.
But I think it belongs in /protocol/ since it's roughly translating things from
the .protos...

http://codereview.chromium.org/7621085/diff/23027/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.h:136: SyncError sync_error);
On 2011/08/25 06:14:06, lipalani1 wrote:
> Ha ha went through the same argument in my head about adding to the
constructor.
> My idea here was to leave it seperate and add a todo to clean up this
structure.
> For example to take this one step further there is no reason why all of this
> should not be a part of SyncerStatus(or a new syncerstatus structure that we
> define). It currently has things like num_commits etc. Things like
> has_more_to_sync, throttled etc should be a part of it as well as should the
> error_counter. I am not fond of making the protocol error part of
error_counter
> even temporarily. But either way I am not passionate on this. So ok to change
if
> you feel there is a compelling reason. Otherwise I can leave a todo here to
> clean up snapshot and file a bug. Currently it is renamed to
> sync_protocol_error.(Also if we go this route, My thinking was this might be a
> good starter bug for new people because it will introduce all the important
> layers of sync. from syncer to ui, in a gentle(!) manner. ).
> On 2011/08/24 14:47:51, timsteele wrote:
> > I think this belongs in SyncerStatus or ErrorCounters, probably the latter
> > though I agree the naming is not ideal.  but snapshot.errors and
> > snapshot.sync_error smells bad, as does adding yet-another-parameter to this
> > constructor. The structs above are intended to reduce the impact of adding
> > things here.
> > 
> > I would add it to ErrorCounters and rename that to just Errors or ErrorState
> > (browser_sync::sessions::ErrorState).  You can add a TODO for the latter.
> 

I read this as "well, yes, this sucks, and we should fix it later, but in the
meantime I'll make it worse and add a todo" :)  Sometimes that's a fine argument
and I've definitely used it myself! But in this case I think there's a better
way that is easy.

I think having snap.errors and snap.sync_protocol_error is the most confusing of
the options we have. So I think it belongs in ErrorCounters, e.g. as
errors.protocol_error.  Whether you want to rename ErrorCounters now is up to
you.

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1134: SVLOG(2) <<
"OnRequestEarlyExit";
nit - log should be updated "SyncScheduler requesting early exit."

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1136: 
nit - extra newline

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1140: 
nit - extra newline.

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_protocol_error.h (right):

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_protocol_error.h:47: UPGRADE_CLIENT,
Each case here should have a comment.

[lipalani1, 2011-08-26 20:07:18]

Please review. All feedback fixed.

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1134: SVLOG(2) <<
"OnRequestEarlyExit";
On 2011/08/25 15:58:55, timsteele wrote:
> nit - log should be updated "SyncScheduler requesting early exit."

Done.

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1136: 
On 2011/08/25 15:58:55, timsteele wrote:
> nit - extra newline

Done.

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:1140: 
On 2011/08/25 15:58:55, timsteele wrote:
> nit - extra newline.

Done.

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_protocol_error.h (right):

http://codereview.chromium.org/7621085/diff/34001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_protocol_error.h:47: UPGRADE_CLIENT,
On 2011/08/25 15:58:55, timsteele wrote:
> Each case here should have a comment.

Done.

[tim (not reviewing), 2011-08-29 23:02:49]

Please remove 'protocol' namespace as discussed, then LGTM

[commit-bot: I haz the power, 2011-08-31 01:46:13]

Try job failure for 7621085-45001 on linux for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-08-31 21:56:40]

Change committed as 99051