Retry: sync: Gracefully handle early shutdown

sync: Gracefully handle early shutdown

Introduce a new object to communicate cross-thread cancellation signals.
This new object, the CancellationSignal, is protected by a lock. It
allows the receiving thread to query whether or not a stop has been
requested. It also allows the receiving thread to safely register a
cross-thread callback to be invoked immediately when a stop is
requested.

We use two instances of this class to ensure we meet all the
requirements for a safe and fast sync backend shutdown.

The first instance is used with the HttpBridgeFactory to allow the UI
thread to force it to drop all refereces to its RequestContextGetter
immediately.  This is an important part of our plan to ensure that all
references to that object are released before
ProfileSyncService::Shutdown() returns, which is necessary to avoid
racy crashes at shutdown.  (See crbug.com/236451)

The second instance is used with the ServerConnectionManager and the
Syncer.  Once signalled, it ensures that any active connections are
released (possibly decrementing another ref to the
RequestContextGetter), that any blocking I/O is aborted, and that no
more connections will be instantiated.  It's important to prevent the
creation of more connections because the HttpBridgeFactory might trigger
a crash if we asked it to create another connection after it had been
shut down.

The syncer's interaction with the second cancelation signal is more
passive.  It does not execute any callbacks when the signal is sent.
Instead, it queries the signal as it performs sync cycles, and will cut
short any existing sync cycle if it notices the signal has been sent.

Finally, this CL includes one important change to the initialization of
the HttpBridgeFactory.  In order to properly register with the
cancellation signal while still allowing the creation of the user agent
to occur on the sync thread, its initialization had to be split into two
parts.  This class now has an Init() method in addition to its
constructor.

BUG=236451
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224014

[rlarocque, 2013-09-10 19:16:00]

Tim, Haitao: PTAL.

This is a retry of https://chromiumcodereview.appspot.com/23189021.

That CL was reverted because there was a race condition at shutdown.  I had
designed the CancelationSignal mechanism at a time when the SyncBackendHost was
guaranteed to outlive its Core, and neglected to update the design to take
lock-free shutdown into account.  The CL caused crashes if the SBH (and its
CancelationSignal member) were destroyed before the SyncScheduler, because it
has an unsafe reference to the CancelationSignal.

This CL tries to fix the issue by making the ownership of CancelationSignal
follow that of the SBH::Core more closely during shutdown.  I've uploaded a copy
of this patch to the old code review, so you can see what's new in this CL by
looking at patch set #8 in https://chromiumcodereview.appspot.com/23189021.

[haitaol1, 2013-09-10 20:36:27]

https://codereview.chromium.org/23717047/diff/1/sync/internal_api/public/sync...
File sync/internal_api/public/sync_manager.h (left):

https://codereview.chromium.org/23717047/diff/1/sync/internal_api/public/sync...
sync/internal_api/public/sync_manager.h:404: virtual void
StopSyncingForShutdown() = 0;
22901013 depends on this method to be called on frontend to release
RequestContextGetter held by HttpBridge and HttpBridgeFactory immediately when
IO loop is still running.

[tim (not reviewing), 2013-09-11 16:21:54]

On 2013/09/10 20:36:27, haitaol1 wrote:
>
https://codereview.chromium.org/23717047/diff/1/sync/internal_api/public/sync...
> File sync/internal_api/public/sync_manager.h (left):
> 
>
https://codereview.chromium.org/23717047/diff/1/sync/internal_api/public/sync...
> sync/internal_api/public/sync_manager.h:404: virtual void
> StopSyncingForShutdown() = 0;
> 22901013 depends on this method to be called on frontend to release
> RequestContextGetter held by HttpBridge and HttpBridgeFactory immediately when
> IO loop is still running.

(I'm going to hold off here until Haitao's concerns are addressed.)

[rlarocque, 2013-09-12 23:53:13]

It's not going to win any beauty contests, but I think this patch can fix all
races related to crbug.com/236451 (assuming I didn't mess up the
implementation).

PTAL.

[haitaol1, 2013-09-13 18:43:21]

https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.h (right):

https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:592:
scoped_ptr<syncer::CancelationSignal> factory_cancelation_signal_;
Maybe move these into SyncBackendRegistrar, which is the last to be destroyed.

[rlarocque, 2013-09-13 20:27:17]

https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.h (right):

https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:592:
scoped_ptr<syncer::CancelationSignal> factory_cancelation_signal_;
On 2013/09/13 18:43:21, haitaol1 wrote:
> Maybe move these into SyncBackendRegistrar, which is the last to be destroyed.

That should work.  However, there is a comment that describes
SyncBackendRegistrar as follows:

// A class that keep track of the workers, change processors, and
// routing info for the enabled sync types, and also routes change
// events to the right processors.

Based on that description, it's hard to justify why such a class should own
these cancelation signals.

Maybe there should be a separate object that owns these signals.  But I don't
think that class should be the one called "SyncBackendRegistrar".

[haitaol1, 2013-09-13 22:18:36]

lgtm

[tim (not reviewing), 2013-09-16 18:30:11]

https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:206:
scoped_ptr<syncer::CancelationSignal> signal1,
Explain here why we pass ownership of these signals.

[rlarocque, 2013-09-16 18:39:09]

Patch updated.

https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:206:
scoped_ptr<syncer::CancelationSignal> signal1,
On 2013/09/16 18:30:11, timsteele wrote:
> Explain here why we pass ownership of these signals.

It's explained in a comment near where this function is invoked. I've added a
short comment here that references the comment in SyncBackendHost::Shutdown().

[tim (not reviewing), 2013-09-16 19:47:12]

On 2013/09/16 18:39:09, rlarocque wrote:
> Patch updated.
> 
>
https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
https://codereview.chromium.org/23717047/diff/25001/chrome/browser/sync/glue/...
> chrome/browser/sync/glue/sync_backend_host.cc:206:
> scoped_ptr<syncer::CancelationSignal> signal1,
> On 2013/09/16 18:30:11, timsteele wrote:
> > Explain here why we pass ownership of these signals.
> 
> It's explained in a comment near where this function is invoked. I've added a
> short comment here that references the comment in SyncBackendHost::Shutdown().

The part that isn't so clear is that you're effectively passing ownership to the
Core here, which 
makes you wonder why we don't just pick the right object at creation or earlier.
i.e, they seem to
live in the wrong place until the end. Having them live in the Registrar might
be easier to follow from
a lifetime perspective, but does seem out of place. 

Explicitly transferring ownership like you have should work. I just think it's
more of a general problem,
and it would be nice to make it easier than landing a bug and debugging a crash
to remember that the core
(and registrar) can outlive the host. Do we have any general comment to that
effect at least?

[tim (not reviewing), 2013-09-16 20:01:35]

tl;dr was I think this change LGTM, but if you can think of a better way to
organize state that needs to live beyond the SBH that might be helpful going
forward.

[rlarocque, 2013-09-16 21:26:57]

On 2013/09/16 20:01:35, timsteele wrote:
> tl;dr was I think this change LGTM, but if you can think of a better way to
> organize state that needs to live beyond the SBH that might be helpful going
> forward.

I think you're on to something.  We can move the signals into the SBH::Core.

I've updated the patch.  PTAL.

[tim (not reviewing), 2013-09-16 23:01:30]

Ah, this looks nicer.  I'll take one more look after comments addressed.

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:243: return
&factory_cancelation_signal_;
|factory| in the name might not be the best choice, at first I thought this was
regarding the ComponentsFactory.  network_cancelation_signal or
http_cancelation_signal?

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:1165: &scm_cancelation_signal_);
Hm, don't we pass this to the syncer? (yet it's called "scm cancelation
signal")?

https://codereview.chromium.org/23717047/diff/39001/sync/internal_api/public/...
File sync/internal_api/public/sync_manager.h (right):

https://codereview.chromium.org/23717047/diff/39001/sync/internal_api/public/...
sync/internal_api/public/sync_manager.h:325: CancelationSignal*
cancelation_signal) = 0;
Comment param in list above.

[rlarocque, 2013-09-17 00:03:21]

Updated.

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:243: return
&factory_cancelation_signal_;
On 2013/09/16 23:01:31, timsteele wrote:
> |factory| in the name might not be the best choice, at first I thought this
was
> regarding the ComponentsFactory.  network_cancelation_signal or
> http_cancelation_signal?

What about the UrlRequestContextGetter?  That object is the real reason we need
all this plumbing.  

request_context_cancelation_signal_?

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:1165: &scm_cancelation_signal_);
On 2013/09/16 23:01:31, timsteele wrote:
> Hm, don't we pass this to the syncer? (yet it's called "scm cancelation
> signal")?

Syncer and SCM, yes.  This signal is the descendant of the one CancelationSignal
that existed before I rebased on top of Haitao's patch.  

The way I think of it is that the SCM is the primary user of this signal, and
the Syncer just happens to check it every now and then, too.  The Syncer's early
exit is best-effort, so it would be OK to use either of the two signals.  This
one gets posted first, so it's slightly better to use it.   

Can you think of a better name for it?  Would scm_and_syncer_cancelation_signal
be an improvement?

https://codereview.chromium.org/23717047/diff/39001/sync/internal_api/public/...
File sync/internal_api/public/sync_manager.h (right):

https://codereview.chromium.org/23717047/diff/39001/sync/internal_api/public/...
sync/internal_api/public/sync_manager.h:325: CancelationSignal*
cancelation_signal) = 0;
On 2013/09/16 23:01:31, timsteele wrote:
> Comment param in list above.

Done.

[tim (not reviewing), 2013-09-17 21:56:40]

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:243: return
&factory_cancelation_signal_;
On 2013/09/17 00:03:22, rlarocque wrote:
> On 2013/09/16 23:01:31, timsteele wrote:
> > |factory| in the name might not be the best choice, at first I thought this
> was
> > regarding the ComponentsFactory.  network_cancelation_signal or
> > http_cancelation_signal?
> 
> What about the UrlRequestContextGetter?  That object is the real reason we
need
> all this plumbing.  
> 
> request_context_cancelation_signal_?

Hm.  That's more confusing I think. 
http_bridge_construction_signal_.RequestStop seems better. 

Also can you explain why we need two?  It's not obvious from looking at the
code.  If we just need one I'd probably call it syncer_cancelation_signal_ or
syncer_stop_requested_signal.  I think those are better names for the second one
(currently called scm_cancelation_signal) because they're more clear in
describing the desired outcome and because abbreviations (like scm) are bad in
code variable names (as per style guide, bad for grepping, etc).

[rlarocque, 2013-09-17 22:46:02]

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:243: return
&factory_cancelation_signal_;
On 2013/09/17 21:56:41, timsteele wrote:
> On 2013/09/17 00:03:22, rlarocque wrote:
> > On 2013/09/16 23:01:31, timsteele wrote:
> > > |factory| in the name might not be the best choice, at first I thought
this
> > was
> > > regarding the ComponentsFactory.  network_cancelation_signal or
> > > http_cancelation_signal?
> > 
> > What about the UrlRequestContextGetter?  That object is the real reason we
> need
> > all this plumbing.  
> > 
> > request_context_cancelation_signal_?
> 
> Hm.  That's more confusing I think. 
> http_bridge_construction_signal_.RequestStop seems better. 

I don't think http_bridge_construction_signal_ is right, either.  This signal
won't prevent HttpBridge construction.  Not gracefully, anyway.  We would
probably crash if we tried to construct an HttpBridge after it had been
signalled.

> 
> Also can you explain why we need two?  It's not obvious from looking at the
> code.  If we just need one I'd probably call it syncer_cancelation_signal_ or
> syncer_stop_requested_signal.  I think those are better names for the second
one
> (currently called scm_cancelation_signal) because they're more clear in
> describing the desired outcome and because abbreviations (like scm) are bad in
> code variable names (as per style guide, bad for grepping, etc).
> 

We technically don't need two, but we would need to use some very subtle tricks
to get away with using just one.

If we attempt to create a new HttpBridge after the factory_cancelation_signal_
is signalled, the HttpBridgeFactory will try to access a non-existent request
context.  The solution to that problem is to ensure that the
ServerConnectionManager is shut down first.

We could do this with just one signal and some sort of observer list that
guaranteed to call its observers in a specific order, but I think it's clearer
to use two separate signals.

I touched on this a bit in the comments around line 1165.  I'll add a few more
comments here.

[tim (not reviewing), 2013-09-18 00:37:55]

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:243: return
&factory_cancelation_signal_;
On 2013/09/17 22:46:02, rlarocque wrote:
> On 2013/09/17 21:56:41, timsteele wrote:
> > On 2013/09/17 00:03:22, rlarocque wrote:
> > > On 2013/09/16 23:01:31, timsteele wrote:
> > > > |factory| in the name might not be the best choice, at first I thought
> this
> > > was
> > > > regarding the ComponentsFactory.  network_cancelation_signal or
> > > > http_cancelation_signal?
> > > 
> > > What about the UrlRequestContextGetter?  That object is the real reason we
> > need
> > > all this plumbing.  
> > > 
> > > request_context_cancelation_signal_?
> > 
> > Hm.  That's more confusing I think. 
> > http_bridge_construction_signal_.RequestStop seems better. 
> 
> I don't think http_bridge_construction_signal_ is right, either.  This signal
> won't prevent HttpBridge construction.  Not gracefully, anyway.  We would
> probably crash if we tried to construct an HttpBridge after it had been
> signalled.

I see.  Logically we're just trying to sever ties with the network stack, but
it's awkward from a naming perspective. I guess 'RequestStop' is part of the
problem since signaling this isn't really "stopping" anything.  Except maybe
'hold_request_context_reference_signal_', I just wish we didn't have to be so
low-level...  But maybe that's the best option at this point?

> 
> > 
> > Also can you explain why we need two?  It's not obvious from looking at the
> > code.  If we just need one I'd probably call it syncer_cancelation_signal_
or
> > syncer_stop_requested_signal.  I think those are better names for the second
> one
> > (currently called scm_cancelation_signal) because they're more clear in
> > describing the desired outcome and because abbreviations (like scm) are bad
in
> > code variable names (as per style guide, bad for grepping, etc).
> > 
> 
> We technically don't need two, but we would need to use some very subtle
tricks
> to get away with using just one.
> 
> If we attempt to create a new HttpBridge after the factory_cancelation_signal_
> is signalled, the HttpBridgeFactory will try to access a non-existent request
> context.  The solution to that problem is to ensure that the
> ServerConnectionManager is shut down first.
> 
> We could do this with just one signal and some sort of observer list that
> guaranteed to call its observers in a specific order, but I think it's clearer
> to use two separate signals.
> 
> I touched on this a bit in the comments around line 1165.  I'll add a few more
> comments here.

That makes sense.  Two definitely seems like the right approach.

[rlarocque, 2013-09-18 00:45:08]

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:243: return
&factory_cancelation_signal_;
On 2013/09/18 00:37:56, timsteele wrote:
> On 2013/09/17 22:46:02, rlarocque wrote:
> > On 2013/09/17 21:56:41, timsteele wrote:
> > > On 2013/09/17 00:03:22, rlarocque wrote:
> > > > On 2013/09/16 23:01:31, timsteele wrote:
> > > > > |factory| in the name might not be the best choice, at first I thought
> > this
> > > > was
> > > > > regarding the ComponentsFactory.  network_cancelation_signal or
> > > > > http_cancelation_signal?
> > > > 
> > > > What about the UrlRequestContextGetter?  That object is the real reason
we
> > > need
> > > > all this plumbing.  
> > > > 
> > > > request_context_cancelation_signal_?
> > > 
> > > Hm.  That's more confusing I think. 
> > > http_bridge_construction_signal_.RequestStop seems better. 
> > 
> > I don't think http_bridge_construction_signal_ is right, either.  This
signal
> > won't prevent HttpBridge construction.  Not gracefully, anyway.  We would
> > probably crash if we tried to construct an HttpBridge after it had been
> > signalled.
> 
> I see.  Logically we're just trying to sever ties with the network stack, but
> it's awkward from a naming perspective. I guess 'RequestStop' is part of the
> problem since signaling this isn't really "stopping" anything.  Except maybe
> 'hold_request_context_reference_signal_', I just wish we didn't have to be so
> low-level...  But maybe that's the best option at this point?
> 

That's not bad.  Maybe we should make their names more verb-like:
- release_request_context_signal_
- stop_syncing_signal_

> > 
> > > 
> > > Also can you explain why we need two?  It's not obvious from looking at
the
> > > code.  If we just need one I'd probably call it syncer_cancelation_signal_
> or
> > > syncer_stop_requested_signal.  I think those are better names for the
second
> > one
> > > (currently called scm_cancelation_signal) because they're more clear in
> > > describing the desired outcome and because abbreviations (like scm) are
bad
> in
> > > code variable names (as per style guide, bad for grepping, etc).
> > > 
> > 
> > We technically don't need two, but we would need to use some very subtle
> tricks
> > to get away with using just one.
> > 
> > If we attempt to create a new HttpBridge after the
factory_cancelation_signal_
> > is signalled, the HttpBridgeFactory will try to access a non-existent
request
> > context.  The solution to that problem is to ensure that the
> > ServerConnectionManager is shut down first.
> > 
> > We could do this with just one signal and some sort of observer list that
> > guaranteed to call its observers in a specific order, but I think it's
clearer
> > to use two separate signals.
> > 
> > I touched on this a bit in the comments around line 1165.  I'll add a few
more
> > comments here.
> 
> That makes sense.  Two definitely seems like the right approach.

[tim (not reviewing), 2013-09-18 01:41:50]

On 2013/09/18 00:45:08, rlarocque wrote:
>
https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
> chrome/browser/sync/glue/sync_backend_host.cc:243: return
> &factory_cancelation_signal_;
> On 2013/09/18 00:37:56, timsteele wrote:
> > On 2013/09/17 22:46:02, rlarocque wrote:
> > > On 2013/09/17 21:56:41, timsteele wrote:
> > > > On 2013/09/17 00:03:22, rlarocque wrote:
> > > > > On 2013/09/16 23:01:31, timsteele wrote:
> > > > > > |factory| in the name might not be the best choice, at first I
thought
> > > this
> > > > > was
> > > > > > regarding the ComponentsFactory.  network_cancelation_signal or
> > > > > > http_cancelation_signal?
> > > > > 
> > > > > What about the UrlRequestContextGetter?  That object is the real
reason
> we
> > > > need
> > > > > all this plumbing.  
> > > > > 
> > > > > request_context_cancelation_signal_?
> > > > 
> > > > Hm.  That's more confusing I think. 
> > > > http_bridge_construction_signal_.RequestStop seems better. 
> > > 
> > > I don't think http_bridge_construction_signal_ is right, either.  This
> signal
> > > won't prevent HttpBridge construction.  Not gracefully, anyway.  We would
> > > probably crash if we tried to construct an HttpBridge after it had been
> > > signalled.
> > 
> > I see.  Logically we're just trying to sever ties with the network stack,
but
> > it's awkward from a naming perspective. I guess 'RequestStop' is part of the
> > problem since signaling this isn't really "stopping" anything.  Except maybe
> > 'hold_request_context_reference_signal_', I just wish we didn't have to be
so
> > low-level...  But maybe that's the best option at this point?
> > 
> 
> That's not bad.  Maybe we should make their names more verb-like:
> - release_request_context_signal_
> - stop_syncing_signal_
> 
> > > 
> > > > 
> > > > Also can you explain why we need two?  It's not obvious from looking at
> the
> > > > code.  If we just need one I'd probably call it
syncer_cancelation_signal_
> > or
> > > > syncer_stop_requested_signal.  I think those are better names for the
> second
> > > one
> > > > (currently called scm_cancelation_signal) because they're more clear in
> > > > describing the desired outcome and because abbreviations (like scm) are
> bad
> > in
> > > > code variable names (as per style guide, bad for grepping, etc).
> > > > 
> > > 
> > > We technically don't need two, but we would need to use some very subtle
> > tricks
> > > to get away with using just one.
> > > 
> > > If we attempt to create a new HttpBridge after the
> factory_cancelation_signal_
> > > is signalled, the HttpBridgeFactory will try to access a non-existent
> request
> > > context.  The solution to that problem is to ensure that the
> > > ServerConnectionManager is shut down first.
> > > 
> > > We could do this with just one signal and some sort of observer list that
> > > guaranteed to call its observers in a specific order, but I think it's
> clearer
> > > to use two separate signals.
> > > 
> > > I touched on this a bit in the comments around line 1165.  I'll add a few
> more
> > > comments here.
> > 
> > That makes sense.  Two definitely seems like the right approach.

Yeah I like making it more verb like, the only problem I was struggling with was
not sounding contrarian to what was actually happening when you call
RequestStop, i.e, release_request_context_signal_.RequestStop ... which is what
led me to have the name match what happens until you RequestStop and go with
hold_request_context. 

But that's maybe less of a problem.  I'll likely go along with whatever you
choose at this point or we can discuss it tomorrow in person when I'm back in
Seattle.

[rlarocque, 2013-09-18 18:34:12]

On 2013/09/18 01:41:50, timsteele wrote:
> On 2013/09/18 00:45:08, rlarocque wrote:
> >
>
https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
> > File chrome/browser/sync/glue/sync_backend_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
> > chrome/browser/sync/glue/sync_backend_host.cc:243: return
> > &factory_cancelation_signal_;
> > On 2013/09/18 00:37:56, timsteele wrote:
> > > On 2013/09/17 22:46:02, rlarocque wrote:
> > > > On 2013/09/17 21:56:41, timsteele wrote:
> > > > > On 2013/09/17 00:03:22, rlarocque wrote:
> > > > > > On 2013/09/16 23:01:31, timsteele wrote:
> > > > > > > |factory| in the name might not be the best choice, at first I
> thought
> > > > this
> > > > > > was
> > > > > > > regarding the ComponentsFactory.  network_cancelation_signal or
> > > > > > > http_cancelation_signal?
> > > > > > 
> > > > > > What about the UrlRequestContextGetter?  That object is the real
> reason
> > we
> > > > > need
> > > > > > all this plumbing.  
> > > > > > 
> > > > > > request_context_cancelation_signal_?
> > > > > 
> > > > > Hm.  That's more confusing I think. 
> > > > > http_bridge_construction_signal_.RequestStop seems better. 
> > > > 
> > > > I don't think http_bridge_construction_signal_ is right, either.  This
> > signal
> > > > won't prevent HttpBridge construction.  Not gracefully, anyway.  We
would
> > > > probably crash if we tried to construct an HttpBridge after it had been
> > > > signalled.
> > > 
> > > I see.  Logically we're just trying to sever ties with the network stack,
> but
> > > it's awkward from a naming perspective. I guess 'RequestStop' is part of
the
> > > problem since signaling this isn't really "stopping" anything.  Except
maybe
> > > 'hold_request_context_reference_signal_', I just wish we didn't have to be
> so
> > > low-level...  But maybe that's the best option at this point?
> > > 
> > 
> > That's not bad.  Maybe we should make their names more verb-like:
> > - release_request_context_signal_
> > - stop_syncing_signal_
> > 
> > > > 
> > > > > 
> > > > > Also can you explain why we need two?  It's not obvious from looking
at
> > the
> > > > > code.  If we just need one I'd probably call it
> syncer_cancelation_signal_
> > > or
> > > > > syncer_stop_requested_signal.  I think those are better names for the
> > second
> > > > one
> > > > > (currently called scm_cancelation_signal) because they're more clear
in
> > > > > describing the desired outcome and because abbreviations (like scm)
are
> > bad
> > > in
> > > > > code variable names (as per style guide, bad for grepping, etc).
> > > > > 
> > > > 
> > > > We technically don't need two, but we would need to use some very subtle
> > > tricks
> > > > to get away with using just one.
> > > > 
> > > > If we attempt to create a new HttpBridge after the
> > factory_cancelation_signal_
> > > > is signalled, the HttpBridgeFactory will try to access a non-existent
> > request
> > > > context.  The solution to that problem is to ensure that the
> > > > ServerConnectionManager is shut down first.
> > > > 
> > > > We could do this with just one signal and some sort of observer list
that
> > > > guaranteed to call its observers in a specific order, but I think it's
> > clearer
> > > > to use two separate signals.
> > > > 
> > > > I touched on this a bit in the comments around line 1165.  I'll add a
few
> > more
> > > > comments here.
> > > 
> > > That makes sense.  Two definitely seems like the right approach.
> 
> Yeah I like making it more verb like, the only problem I was struggling with
was
> not sounding contrarian to what was actually happening when you call
> RequestStop, i.e, release_request_context_signal_.RequestStop ... which is
what
> led me to have the name match what happens until you RequestStop and go with
> hold_request_context. 
> 
> But that's maybe less of a problem.  I'll likely go along with whatever you
> choose at this point or we can discuss it tomorrow in person when I'm back in
> Seattle.

Good point.  Let's s/RequestStop()/Signal()/, verbify the names, and call it
good enough?

[rlarocque, 2013-09-18 19:08:11]

On 2013/09/18 18:34:12, rlarocque wrote:
> On 2013/09/18 01:41:50, timsteele wrote:
> > On 2013/09/18 00:45:08, rlarocque wrote:
> > >
> >
>
https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
> > > File chrome/browser/sync/glue/sync_backend_host.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23717047/diff/39001/chrome/browser/sync/glue/...
> > > chrome/browser/sync/glue/sync_backend_host.cc:243: return
> > > &factory_cancelation_signal_;
> > > On 2013/09/18 00:37:56, timsteele wrote:
> > > > On 2013/09/17 22:46:02, rlarocque wrote:
> > > > > On 2013/09/17 21:56:41, timsteele wrote:
> > > > > > On 2013/09/17 00:03:22, rlarocque wrote:
> > > > > > > On 2013/09/16 23:01:31, timsteele wrote:
> > > > > > > > |factory| in the name might not be the best choice, at first I
> > thought
> > > > > this
> > > > > > > was
> > > > > > > > regarding the ComponentsFactory.  network_cancelation_signal or
> > > > > > > > http_cancelation_signal?
> > > > > > > 
> > > > > > > What about the UrlRequestContextGetter?  That object is the real
> > reason
> > > we
> > > > > > need
> > > > > > > all this plumbing.  
> > > > > > > 
> > > > > > > request_context_cancelation_signal_?
> > > > > > 
> > > > > > Hm.  That's more confusing I think. 
> > > > > > http_bridge_construction_signal_.RequestStop seems better. 
> > > > > 
> > > > > I don't think http_bridge_construction_signal_ is right, either.  This
> > > signal
> > > > > won't prevent HttpBridge construction.  Not gracefully, anyway.  We
> would
> > > > > probably crash if we tried to construct an HttpBridge after it had
been
> > > > > signalled.
> > > > 
> > > > I see.  Logically we're just trying to sever ties with the network
stack,
> > but
> > > > it's awkward from a naming perspective. I guess 'RequestStop' is part of
> the
> > > > problem since signaling this isn't really "stopping" anything.  Except
> maybe
> > > > 'hold_request_context_reference_signal_', I just wish we didn't have to
be
> > so
> > > > low-level...  But maybe that's the best option at this point?
> > > > 
> > > 
> > > That's not bad.  Maybe we should make their names more verb-like:
> > > - release_request_context_signal_
> > > - stop_syncing_signal_
> > > 
> > > > > 
> > > > > > 
> > > > > > Also can you explain why we need two?  It's not obvious from looking
> at
> > > the
> > > > > > code.  If we just need one I'd probably call it
> > syncer_cancelation_signal_
> > > > or
> > > > > > syncer_stop_requested_signal.  I think those are better names for
the
> > > second
> > > > > one
> > > > > > (currently called scm_cancelation_signal) because they're more clear
> in
> > > > > > describing the desired outcome and because abbreviations (like scm)
> are
> > > bad
> > > > in
> > > > > > code variable names (as per style guide, bad for grepping, etc).
> > > > > > 
> > > > > 
> > > > > We technically don't need two, but we would need to use some very
subtle
> > > > tricks
> > > > > to get away with using just one.
> > > > > 
> > > > > If we attempt to create a new HttpBridge after the
> > > factory_cancelation_signal_
> > > > > is signalled, the HttpBridgeFactory will try to access a non-existent
> > > request
> > > > > context.  The solution to that problem is to ensure that the
> > > > > ServerConnectionManager is shut down first.
> > > > > 
> > > > > We could do this with just one signal and some sort of observer list
> that
> > > > > guaranteed to call its observers in a specific order, but I think it's
> > > clearer
> > > > > to use two separate signals.
> > > > > 
> > > > > I touched on this a bit in the comments around line 1165.  I'll add a
> few
> > > more
> > > > > comments here.
> > > > 
> > > > That makes sense.  Two definitely seems like the right approach.
> > 
> > Yeah I like making it more verb like, the only problem I was struggling with
> was
> > not sounding contrarian to what was actually happening when you call
> > RequestStop, i.e, release_request_context_signal_.RequestStop ... which is
> what
> > led me to have the name match what happens until you RequestStop and go with
> > hold_request_context. 
> > 
> > But that's maybe less of a problem.  I'll likely go along with whatever you
> > choose at this point or we can discuss it tomorrow in person when I'm back
in
> > Seattle.
> 
> Good point.  Let's s/RequestStop()/Signal()/, verbify the names, and call it
> good enough?

Rename uploaded.  PTAL.

[tim (not reviewing), 2013-09-18 20:00:46]

Yeah, that sounds good. LGTM

[commit-bot: I haz the power, 2013-09-18 20:08:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/23717047/59001

[commit-bot: I haz the power, 2013-09-19 01:17:04]

Change committed as 224014