Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698723003:
chrome[android]: Restrict the use of startService for invalidations. (Closed)
Descriptionchrome[android]: Restrict the use of startService for invalidations.
The change restricts the use of background services when the application
is not in foreground for N+. In these cases we simply don't perform the
operation since the chain of services finally needs to start a service
in the cache invalidation library, which uses the same fallback.
BUG=680812
Review-Url: https://codereview.chromium.org/2698723003
Cr-Commit-Position: refs/heads/master@{#452277}
Committed: https://chromium.googlesource.com/chromium/src/+/090c3e70b9886ec428420dd6b3b0011907b61e16
Patch Set 1 #
Total comments: 11
Messages
Total messages: 24 (6 generated)
khushalsagar@chromium.org changed reviewers: + nyquist@chromium.org
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: if (shouldRestrictBackgroundServices()) { Sigh. I considered this one in particular, since we persist the current registrations in InvalidationPreferences and then use the diff to figure out which objects to register and unregister. But eventually the InvalidationClientService is going to call into the AndroidListener in cache invalidation which can also fail, leaving us in an inconsistent state. Though when I dug around a bit more, looks like ensureStartedAndUpdateRegisteredTypes in InvalidationController will do this update also. Would it be expensive to do that each time the application state changes to foreground for this restrict case?
nyquist@chromium.org changed reviewers: + ghc@google.com
ghc: Want to take a first pass at this to ensure we are using the APIs correctly?
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: if (shouldRestrictBackgroundServices()) { On 2017/02/16 05:34:23, Khushal wrote: > Sigh. I considered this one in particular, since we persist the current > registrations in InvalidationPreferences and then use the diff to figure out > which objects to register and unregister. But eventually the > InvalidationClientService is going to call into the AndroidListener in cache > invalidation which can also fail, leaving us in an inconsistent state. > > Though when I dug around a bit more, looks like > ensureStartedAndUpdateRegisteredTypes in InvalidationController will do this > update also. Would it be expensive to do that each time the application state > changes to foreground for this restrict case? I think that would involve sending all registrations every time the application goes to the foreground. That would be significantly more load on the servers and probably not feasible. A better approach might be to keep track of whether the latest registration state has been communicated to the ticl, and if not, try to do so upon switching to foreground. Would something like that be hard to implement?
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: if (shouldRestrictBackgroundServices()) { On 2017/02/17 19:27:38, ghc wrote: > On 2017/02/16 05:34:23, Khushal wrote: > > Sigh. I considered this one in particular, since we persist the current > > registrations in InvalidationPreferences and then use the diff to figure out > > which objects to register and unregister. But eventually the > > InvalidationClientService is going to call into the AndroidListener in cache > > invalidation which can also fail, leaving us in an inconsistent state. > > > > Though when I dug around a bit more, looks like > > ensureStartedAndUpdateRegisteredTypes in InvalidationController will do this > > update also. Would it be expensive to do that each time the application state > > changes to foreground for this restrict case? > > I think that would involve sending all registrations every time the application > goes to the foreground. That would be significantly more load on the servers and > probably not feasible. A better approach might be to keep track of whether the > latest registration state has been communicated to the ticl, and if not, try to > do so upon switching to foreground. Would something like that be hard to > implement? That's what the current code tries to do I think. Here we store the registrations communicated to Ticl in SharedPrefs and only give the diff when something changes: https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav... But given the fact that this request can fail later down the stack in the service hops before going to Ticl, I don't know whether this would work. Does TiclState try to do some internal tracking for only communicating new registrations to the server?
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: if (shouldRestrictBackgroundServices()) { On 2017/02/17 21:12:01, Khushal wrote: > On 2017/02/17 19:27:38, ghc wrote: > > On 2017/02/16 05:34:23, Khushal wrote: > > > Sigh. I considered this one in particular, since we persist the current > > > registrations in InvalidationPreferences and then use the diff to figure out > > > which objects to register and unregister. But eventually the > > > InvalidationClientService is going to call into the AndroidListener in cache > > > invalidation which can also fail, leaving us in an inconsistent state. > > > > > > Though when I dug around a bit more, looks like > > > ensureStartedAndUpdateRegisteredTypes in InvalidationController will do this > > > update also. Would it be expensive to do that each time the application > state > > > changes to foreground for this restrict case? > > > > I think that would involve sending all registrations every time the > application > > goes to the foreground. That would be significantly more load on the servers > and > > probably not feasible. A better approach might be to keep track of whether the > > latest registration state has been communicated to the ticl, and if not, try > to > > do so upon switching to foreground. Would something like that be hard to > > implement? > > That's what the current code tries to do I think. Here we store the > registrations communicated to Ticl in SharedPrefs and only give the diff when > something changes: > https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav... > > But given the fact that this request can fail later down the stack in the > service hops before going to Ticl, I don't know whether this would work. Does > TiclState try to do some internal tracking for only communicating new > registrations to the server? There is logic to avoid sending a message if the set of registered objects hasn't changed (see bottom of InvalidationClientCore::performRegisterOperations). I assume this should work in such a scenario, but I'm not 100% certain. (The Ticl also delivers a registration status callback, and for a new registration you'll always receive an initial invalidation. If you receive an invalidation for an object, you can assume that a registration succeeded; that could be another signal to avoid unnecessary re-registration.)
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: if (shouldRestrictBackgroundServices()) { On 2017/02/17 21:31:36, ghc wrote: > On 2017/02/17 21:12:01, Khushal wrote: > > On 2017/02/17 19:27:38, ghc wrote: > > > On 2017/02/16 05:34:23, Khushal wrote: > > > > Sigh. I considered this one in particular, since we persist the current > > > > registrations in InvalidationPreferences and then use the diff to figure > out > > > > which objects to register and unregister. But eventually the > > > > InvalidationClientService is going to call into the AndroidListener in > cache > > > > invalidation which can also fail, leaving us in an inconsistent state. > > > > > > > > Though when I dug around a bit more, looks like > > > > ensureStartedAndUpdateRegisteredTypes in InvalidationController will do > this > > > > update also. Would it be expensive to do that each time the application > > state > > > > changes to foreground for this restrict case? > > > > > > I think that would involve sending all registrations every time the > > application > > > goes to the foreground. That would be significantly more load on the servers > > and > > > probably not feasible. A better approach might be to keep track of whether > the > > > latest registration state has been communicated to the ticl, and if not, try > > to > > > do so upon switching to foreground. Would something like that be hard to > > > implement? > > > > That's what the current code tries to do I think. Here we store the > > registrations communicated to Ticl in SharedPrefs and only give the diff when > > something changes: > > > https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav... > > > > But given the fact that this request can fail later down the stack in the > > service hops before going to Ticl, I don't know whether this would work. Does > > TiclState try to do some internal tracking for only communicating new > > registrations to the server? > > There is logic to avoid sending a message if the set of registered objects > hasn't changed (see bottom of > InvalidationClientCore::performRegisterOperations). I assume this should work in > such a scenario, but I'm not 100% certain. > > (The Ticl also delivers a registration status callback, and for a new > registration you'll always receive an initial invalidation. If you receive an > invalidation for an object, you can assume that a registration succeeded; that > could be another signal to avoid unnecessary re-registration.) > What about this code: https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c... AndroidListener has persisted state for what has been registered, and is updated and written to disk after each intent. So if we simply re-issue the registration request, this code should take care of only sending a message to the server only if there are new registrations, right? In fact that makes me wonder why we store this in InvalidationPrefs at all in chrome. Am I missing something?
On 2017/02/17 21:54:52, Khushal wrote: > https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... > File > components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java > (right): > > https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... > components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: > if (shouldRestrictBackgroundServices()) { > On 2017/02/17 21:31:36, ghc wrote: > > On 2017/02/17 21:12:01, Khushal wrote: > > > On 2017/02/17 19:27:38, ghc wrote: > > > > On 2017/02/16 05:34:23, Khushal wrote: > > > > > Sigh. I considered this one in particular, since we persist the current > > > > > registrations in InvalidationPreferences and then use the diff to figure > > out > > > > > which objects to register and unregister. But eventually the > > > > > InvalidationClientService is going to call into the AndroidListener in > > cache > > > > > invalidation which can also fail, leaving us in an inconsistent state. > > > > > > > > > > Though when I dug around a bit more, looks like > > > > > ensureStartedAndUpdateRegisteredTypes in InvalidationController will do > > this > > > > > update also. Would it be expensive to do that each time the application > > > state > > > > > changes to foreground for this restrict case? > > > > > > > > I think that would involve sending all registrations every time the > > > application > > > > goes to the foreground. That would be significantly more load on the > servers > > > and > > > > probably not feasible. A better approach might be to keep track of whether > > the > > > > latest registration state has been communicated to the ticl, and if not, > try > > > to > > > > do so upon switching to foreground. Would something like that be hard to > > > > implement? > > > > > > That's what the current code tries to do I think. Here we store the > > > registrations communicated to Ticl in SharedPrefs and only give the diff > when > > > something changes: > > > > > > https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav... > > > > > > But given the fact that this request can fail later down the stack in the > > > service hops before going to Ticl, I don't know whether this would work. > Does > > > TiclState try to do some internal tracking for only communicating new > > > registrations to the server? > > > > There is logic to avoid sending a message if the set of registered objects > > hasn't changed (see bottom of > > InvalidationClientCore::performRegisterOperations). I assume this should work > in > > such a scenario, but I'm not 100% certain. > > > > (The Ticl also delivers a registration status callback, and for a new > > registration you'll always receive an initial invalidation. If you receive an > > invalidation for an object, you can assume that a registration succeeded; that > > could be another signal to avoid unnecessary re-registration.) > > > > What about this code: > https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c... > > AndroidListener has persisted state for what has been registered, and is updated > and written to disk after each intent. So if we simply re-issue the registration > request, this code should take care of only sending a message to the server only > if there are new registrations, right? In fact that makes me wonder why we store > this in InvalidationPrefs at all in chrome. Am I missing something? Ah, yes. It looks like AndroidListener[State] tracks this, so we should be safe.
On 2017/02/17 22:33:16, ghc wrote: > On 2017/02/17 21:54:52, Khushal wrote: > > > https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... > > File > > > components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java > > (right): > > > > > https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... > > > components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: > > if (shouldRestrictBackgroundServices()) { > > On 2017/02/17 21:31:36, ghc wrote: > > > On 2017/02/17 21:12:01, Khushal wrote: > > > > On 2017/02/17 19:27:38, ghc wrote: > > > > > On 2017/02/16 05:34:23, Khushal wrote: > > > > > > Sigh. I considered this one in particular, since we persist the > current > > > > > > registrations in InvalidationPreferences and then use the diff to > figure > > > out > > > > > > which objects to register and unregister. But eventually the > > > > > > InvalidationClientService is going to call into the AndroidListener in > > > cache > > > > > > invalidation which can also fail, leaving us in an inconsistent state. > > > > > > > > > > > > Though when I dug around a bit more, looks like > > > > > > ensureStartedAndUpdateRegisteredTypes in InvalidationController will > do > > > this > > > > > > update also. Would it be expensive to do that each time the > application > > > > state > > > > > > changes to foreground for this restrict case? > > > > > > > > > > I think that would involve sending all registrations every time the > > > > application > > > > > goes to the foreground. That would be significantly more load on the > > servers > > > > and > > > > > probably not feasible. A better approach might be to keep track of > whether > > > the > > > > > latest registration state has been communicated to the ticl, and if not, > > try > > > > to > > > > > do so upon switching to foreground. Would something like that be hard to > > > > > implement? > > > > > > > > That's what the current code tries to do I think. Here we store the > > > > registrations communicated to Ticl in SharedPrefs and only give the diff > > when > > > > something changes: > > > > > > > > > > https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav... > > > > > > > > But given the fact that this request can fail later down the stack in the > > > > service hops before going to Ticl, I don't know whether this would work. > > Does > > > > TiclState try to do some internal tracking for only communicating new > > > > registrations to the server? > > > > > > There is logic to avoid sending a message if the set of registered objects > > > hasn't changed (see bottom of > > > InvalidationClientCore::performRegisterOperations). I assume this should > work > > in > > > such a scenario, but I'm not 100% certain. > > > > > > (The Ticl also delivers a registration status callback, and for a new > > > registration you'll always receive an initial invalidation. If you receive > an > > > invalidation for an object, you can assume that a registration succeeded; > that > > > could be another signal to avoid unnecessary re-registration.) > > > > > > > What about this code: > > > https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c... > > > > AndroidListener has persisted state for what has been registered, and is > updated > > and written to disk after each intent. So if we simply re-issue the > registration > > request, this code should take care of only sending a message to the server > only > > if there are new registrations, right? In fact that makes me wonder why we > store > > this in InvalidationPrefs at all in chrome. Am I missing something? > > Ah, yes. It looks like AndroidListener[State] tracks this, so we should be safe. Funny thing is. Now I see the case where the InvalidationClientService could save the registrations in SharedPrefs and then call register, which tries to start the AndroidListener service and fails. So the next time we do ensureStartedAndUpdateRegisteredTypes, the InvalidationClientService will assume that these registrations were already given to Ticl, but they weren't. If Ticl is already tracking state for sending registrations only when something changes, then should we remove the duplicate code from InvalidationClientService#setRegisteredTypes? Then all we need to do is re-issue registrations on coming back to foreground, which shouldn't be expensive because Ticl wouldn't send anything if there was no state change.
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: if (shouldRestrictBackgroundServices()) { On 2017/02/17 21:54:52, Khushal wrote: > On 2017/02/17 21:31:36, ghc wrote: > > On 2017/02/17 21:12:01, Khushal wrote: > > > On 2017/02/17 19:27:38, ghc wrote: > > > > On 2017/02/16 05:34:23, Khushal wrote: > > > > > Sigh. I considered this one in particular, since we persist the current > > > > > registrations in InvalidationPreferences and then use the diff to figure > > out > > > > > which objects to register and unregister. But eventually the > > > > > InvalidationClientService is going to call into the AndroidListener in > > cache > > > > > invalidation which can also fail, leaving us in an inconsistent state. > > > > > > > > > > Though when I dug around a bit more, looks like > > > > > ensureStartedAndUpdateRegisteredTypes in InvalidationController will do > > this > > > > > update also. Would it be expensive to do that each time the application > > > state > > > > > changes to foreground for this restrict case? > > > > > > > > I think that would involve sending all registrations every time the > > > application > > > > goes to the foreground. That would be significantly more load on the > servers > > > and > > > > probably not feasible. A better approach might be to keep track of whether > > the > > > > latest registration state has been communicated to the ticl, and if not, > try > > > to > > > > do so upon switching to foreground. Would something like that be hard to > > > > implement? > > > > > > That's what the current code tries to do I think. Here we store the > > > registrations communicated to Ticl in SharedPrefs and only give the diff > when > > > something changes: > > > > > > https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav... > > > > > > But given the fact that this request can fail later down the stack in the > > > service hops before going to Ticl, I don't know whether this would work. > Does > > > TiclState try to do some internal tracking for only communicating new > > > registrations to the server? > > > > There is logic to avoid sending a message if the set of registered objects > > hasn't changed (see bottom of > > InvalidationClientCore::performRegisterOperations). I assume this should work > in > > such a scenario, but I'm not 100% certain. > > > > (The Ticl also delivers a registration status callback, and for a new > > registration you'll always receive an initial invalidation. If you receive an > > invalidation for an object, you can assume that a registration succeeded; that > > could be another signal to avoid unnecessary re-registration.) > > > > What about this code: > https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c... > > AndroidListener has persisted state for what has been registered, and is updated > and written to disk after each intent. So if we simply re-issue the registration > request, this code should take care of only sending a message to the server only > if there are new registrations, right? In fact that makes me wonder why we store > this in InvalidationPrefs at all in chrome. Am I missing something? So this is a little bit scary. If this ends up failing at some point, we will have persisted incorrect state, and possibly will not ever fix it. Could we instead of the internals of ticl to not ping the server too often?
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: if (shouldRestrictBackgroundServices()) { On 2017/02/21 19:46:18, nyquist wrote: > On 2017/02/17 21:54:52, Khushal wrote: > > On 2017/02/17 21:31:36, ghc wrote: > > > On 2017/02/17 21:12:01, Khushal wrote: > > > > On 2017/02/17 19:27:38, ghc wrote: > > > > > On 2017/02/16 05:34:23, Khushal wrote: > > > > > > Sigh. I considered this one in particular, since we persist the > current > > > > > > registrations in InvalidationPreferences and then use the diff to > figure > > > out > > > > > > which objects to register and unregister. But eventually the > > > > > > InvalidationClientService is going to call into the AndroidListener in > > > cache > > > > > > invalidation which can also fail, leaving us in an inconsistent state. > > > > > > > > > > > > Though when I dug around a bit more, looks like > > > > > > ensureStartedAndUpdateRegisteredTypes in InvalidationController will > do > > > this > > > > > > update also. Would it be expensive to do that each time the > application > > > > state > > > > > > changes to foreground for this restrict case? > > > > > > > > > > I think that would involve sending all registrations every time the > > > > application > > > > > goes to the foreground. That would be significantly more load on the > > servers > > > > and > > > > > probably not feasible. A better approach might be to keep track of > whether > > > the > > > > > latest registration state has been communicated to the ticl, and if not, > > try > > > > to > > > > > do so upon switching to foreground. Would something like that be hard to > > > > > implement? > > > > > > > > That's what the current code tries to do I think. Here we store the > > > > registrations communicated to Ticl in SharedPrefs and only give the diff > > when > > > > something changes: > > > > > > > > > > https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav... > > > > > > > > But given the fact that this request can fail later down the stack in the > > > > service hops before going to Ticl, I don't know whether this would work. > > Does > > > > TiclState try to do some internal tracking for only communicating new > > > > registrations to the server? > > > > > > There is logic to avoid sending a message if the set of registered objects > > > hasn't changed (see bottom of > > > InvalidationClientCore::performRegisterOperations). I assume this should > work > > in > > > such a scenario, but I'm not 100% certain. > > > > > > (The Ticl also delivers a registration status callback, and for a new > > > registration you'll always receive an initial invalidation. If you receive > an > > > invalidation for an object, you can assume that a registration succeeded; > that > > > could be another signal to avoid unnecessary re-registration.) > > > > > > > What about this code: > > > https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c... > > > > AndroidListener has persisted state for what has been registered, and is > updated > > and written to disk after each intent. So if we simply re-issue the > registration > > request, this code should take care of only sending a message to the server > only > > if there are new registrations, right? In fact that makes me wonder why we > store > > this in InvalidationPrefs at all in chrome. Am I missing something? > > So this is a little bit scary. If this ends up failing at some point, we will > have persisted incorrect state, and possibly will not ever fix it. > > Could we instead of the internals of ticl to not ping the server too often? About the discussion on persisted state, on taking a deeper look at where all state is stored I think we can be left with an inconsistent state in the AndroidListener as well. There is InvalidationPreferences being stored here and we can fail to communicate that to the AndroidListener. AndroidListener is further storing it internally as desiredRegistrations and sending them to TiclService which can also fail. And finally Ticl is storing it in the InvalidationClientCore which I think has the final state used for deciding whether server sync is necessary and what to send (correct me if I understood something incorrectly). So while we could make an effort to not store anything in InvalidationPrefs and avoid inconsistency with AndroidListener's state, I don't have a good idea for AndroidListener and TiclState inconsistency.
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: if (shouldRestrictBackgroundServices()) { On 2017/02/21 20:52:25, Khushal wrote: > On 2017/02/21 19:46:18, nyquist wrote: > > On 2017/02/17 21:54:52, Khushal wrote: > > > On 2017/02/17 21:31:36, ghc wrote: > > > > On 2017/02/17 21:12:01, Khushal wrote: > > > > > On 2017/02/17 19:27:38, ghc wrote: > > > > > > On 2017/02/16 05:34:23, Khushal wrote: > > > > > > > Sigh. I considered this one in particular, since we persist the > > current > > > > > > > registrations in InvalidationPreferences and then use the diff to > > figure > > > > out > > > > > > > which objects to register and unregister. But eventually the > > > > > > > InvalidationClientService is going to call into the AndroidListener > in > > > > cache > > > > > > > invalidation which can also fail, leaving us in an inconsistent > state. > > > > > > > > > > > > > > Though when I dug around a bit more, looks like > > > > > > > ensureStartedAndUpdateRegisteredTypes in InvalidationController will > > do > > > > this > > > > > > > update also. Would it be expensive to do that each time the > > application > > > > > state > > > > > > > changes to foreground for this restrict case? > > > > > > > > > > > > I think that would involve sending all registrations every time the > > > > > application > > > > > > goes to the foreground. That would be significantly more load on the > > > servers > > > > > and > > > > > > probably not feasible. A better approach might be to keep track of > > whether > > > > the > > > > > > latest registration state has been communicated to the ticl, and if > not, > > > try > > > > > to > > > > > > do so upon switching to foreground. Would something like that be hard > to > > > > > > implement? > > > > > > > > > > That's what the current code tries to do I think. Here we store the > > > > > registrations communicated to Ticl in SharedPrefs and only give the diff > > > when > > > > > something changes: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav... > > > > > > > > > > But given the fact that this request can fail later down the stack in > the > > > > > service hops before going to Ticl, I don't know whether this would work. > > > Does > > > > > TiclState try to do some internal tracking for only communicating new > > > > > registrations to the server? > > > > > > > > There is logic to avoid sending a message if the set of registered objects > > > > hasn't changed (see bottom of > > > > InvalidationClientCore::performRegisterOperations). I assume this should > > work > > > in > > > > such a scenario, but I'm not 100% certain. > > > > > > > > (The Ticl also delivers a registration status callback, and for a new > > > > registration you'll always receive an initial invalidation. If you receive > > an > > > > invalidation for an object, you can assume that a registration succeeded; > > that > > > > could be another signal to avoid unnecessary re-registration.) > > > > > > > > > > What about this code: > > > > > > https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c... > > > > > > AndroidListener has persisted state for what has been registered, and is > > updated > > > and written to disk after each intent. So if we simply re-issue the > > registration > > > request, this code should take care of only sending a message to the server > > only > > > if there are new registrations, right? In fact that makes me wonder why we > > store > > > this in InvalidationPrefs at all in chrome. Am I missing something? > > > > So this is a little bit scary. If this ends up failing at some point, we will > > have persisted incorrect state, and possibly will not ever fix it. > > > > Could we instead of the internals of ticl to not ping the server too often? > > About the discussion on persisted state, on taking a deeper look at where all > state is stored I think we can be left with an inconsistent state in the > AndroidListener as well. There is InvalidationPreferences being stored here and > we can fail to communicate that to the AndroidListener. AndroidListener is > further storing it internally as desiredRegistrations and sending them to > TiclService which can also fail. And finally Ticl is storing it in the > InvalidationClientCore which I think has the final state used for deciding > whether server sync is necessary and what to send (correct me if I understood > something incorrectly). > > So while we could make an effort to not store anything in InvalidationPrefs and > avoid inconsistency with AndroidListener's state, I don't have a good idea for > AndroidListener and TiclState inconsistency. Well then, unless ghc@ has a better idea, I guess we should just submit this.
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java:80: if (shouldRestrictBackgroundServices()) { On 2017/02/22 10:48:13, nyquist wrote: > On 2017/02/21 20:52:25, Khushal wrote: > > On 2017/02/21 19:46:18, nyquist wrote: > > > On 2017/02/17 21:54:52, Khushal wrote: > > > > On 2017/02/17 21:31:36, ghc wrote: > > > > > On 2017/02/17 21:12:01, Khushal wrote: > > > > > > On 2017/02/17 19:27:38, ghc wrote: > > > > > > > On 2017/02/16 05:34:23, Khushal wrote: > > > > > > > > Sigh. I considered this one in particular, since we persist the > > > current > > > > > > > > registrations in InvalidationPreferences and then use the diff to > > > figure > > > > > out > > > > > > > > which objects to register and unregister. But eventually the > > > > > > > > InvalidationClientService is going to call into the > AndroidListener > > in > > > > > cache > > > > > > > > invalidation which can also fail, leaving us in an inconsistent > > state. > > > > > > > > > > > > > > > > Though when I dug around a bit more, looks like > > > > > > > > ensureStartedAndUpdateRegisteredTypes in InvalidationController > will > > > do > > > > > this > > > > > > > > update also. Would it be expensive to do that each time the > > > application > > > > > > state > > > > > > > > changes to foreground for this restrict case? > > > > > > > > > > > > > > I think that would involve sending all registrations every time the > > > > > > application > > > > > > > goes to the foreground. That would be significantly more load on the > > > > servers > > > > > > and > > > > > > > probably not feasible. A better approach might be to keep track of > > > whether > > > > > the > > > > > > > latest registration state has been communicated to the ticl, and if > > not, > > > > try > > > > > > to > > > > > > > do so upon switching to foreground. Would something like that be > hard > > to > > > > > > > implement? > > > > > > > > > > > > That's what the current code tries to do I think. Here we store the > > > > > > registrations communicated to Ticl in SharedPrefs and only give the > diff > > > > when > > > > > > something changes: > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav... > > > > > > > > > > > > But given the fact that this request can fail later down the stack in > > the > > > > > > service hops before going to Ticl, I don't know whether this would > work. > > > > Does > > > > > > TiclState try to do some internal tracking for only communicating new > > > > > > registrations to the server? > > > > > > > > > > There is logic to avoid sending a message if the set of registered > objects > > > > > hasn't changed (see bottom of > > > > > InvalidationClientCore::performRegisterOperations). I assume this should > > > work > > > > in > > > > > such a scenario, but I'm not 100% certain. > > > > > > > > > > (The Ticl also delivers a registration status callback, and for a new > > > > > registration you'll always receive an initial invalidation. If you > receive > > > an > > > > > invalidation for an object, you can assume that a registration > succeeded; > > > that > > > > > could be another signal to avoid unnecessary re-registration.) > > > > > > > > > > > > > What about this code: > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c... > > > > > > > > AndroidListener has persisted state for what has been registered, and is > > > updated > > > > and written to disk after each intent. So if we simply re-issue the > > > registration > > > > request, this code should take care of only sending a message to the > server > > > only > > > > if there are new registrations, right? In fact that makes me wonder why we > > > store > > > > this in InvalidationPrefs at all in chrome. Am I missing something? > > > > > > So this is a little bit scary. If this ends up failing at some point, we > will > > > have persisted incorrect state, and possibly will not ever fix it. > > > > > > Could we instead of the internals of ticl to not ping the server too often? > > > > About the discussion on persisted state, on taking a deeper look at where all > > state is stored I think we can be left with an inconsistent state in the > > AndroidListener as well. There is InvalidationPreferences being stored here > and > > we can fail to communicate that to the AndroidListener. AndroidListener is > > further storing it internally as desiredRegistrations and sending them to > > TiclService which can also fail. And finally Ticl is storing it in the > > InvalidationClientCore which I think has the final state used for deciding > > whether server sync is necessary and what to send (correct me if I understood > > something incorrectly). > > > > So while we could make an effort to not store anything in InvalidationPrefs > and > > avoid inconsistency with AndroidListener's state, I don't have a good idea for > > AndroidListener and TiclState inconsistency. > > Well then, unless ghc@ has a better idea, I guess we should just submit this. I don't think I have any better ideas. The ticl is a subtle piece of code with some tricky state machine logic. The risk of a code change breaking one of its invariants seems pretty high.
Let's go ahead with this then, tommy?
yup! lgtm
The CQ bit was checked by khushalsagar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1487804252073400, "parent_rev":
"472ee1be969773c6518d44a29c22abdf9f3d2002", "commit_rev":
"090c3e70b9886ec428420dd6b3b0011907b61e16"}
Message was sent while issue was closed.
Description was changed from ========== chrome[android]: Restrict the use of startService for invalidations. The change restricts the use of background services when the application is not in foreground for N+. In these cases we simply don't perform the operation since the chain of services finally needs to start a service in the cache invalidation library, which uses the same fallback. BUG=680812 ========== to ========== chrome[android]: Restrict the use of startService for invalidations. The change restricts the use of background services when the application is not in foreground for N+. In these cases we simply don't perform the operation since the chain of services finally needs to start a service in the cache invalidation library, which uses the same fallback. BUG=680812 Review-Url: https://codereview.chromium.org/2698723003 Cr-Commit-Position: refs/heads/master@{#452277} Committed: https://chromium.googlesource.com/chromium/src/+/090c3e70b9886ec428420dd6b3b0... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/090c3e70b9886ec428420dd6b3b0...
Message was sent while issue was closed.
zea@chromium.org changed reviewers: + zea@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationClientService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationClientService.java:556: return BuildInfo.isGreaterThanN() && !isChromeInForeground(); Looks like this will actually return true when the device is running NMR1 https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... Is that expected? Should this instead be calling isAtLeastO?
Message was sent while issue was closed.
https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... File components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationClientService.java (right): https://codereview.chromium.org/2698723003/diff/1/components/invalidation/imp... components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationClientService.java:556: return BuildInfo.isGreaterThanN() && !isChromeInForeground(); On 2017/03/30 18:39:35, Nicolas Zea wrote: > Looks like this will actually return true when the device is running NMR1 > https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... > > Is that expected? Should this instead be calling isAtLeastO? This looks like an error. It should be isAtLeastO. If we fail to stop the client here, for O tango would stop because android would throw an exception if we try to start a service, but on N the client would keep running. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
