Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2652653007:
Chromad: Refresh policy every 90 minutes (Closed)
|
Created:
3 years, 11 months ago by Thiemo Nagel Modified:
3 years, 10 months ago Reviewers:
emaxx CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromad: Refresh policy every 90 minutes
Applies both to user and device policy.
BUG=677304
Review-Url: https://codereview.chromium.org/2652653007
Cr-Commit-Position: refs/heads/master@{#446665}
Committed: https://chromium.googlesource.com/chromium/src/+/5c9d59322119761d3b2b8569345b7cdd5cec6f2a
Patch Set 1 #Patch Set 2 : Polish #
Total comments: 14
Patch Set 3 : Rebase #Patch Set 4 : Refactoring / comments / improvements #
Total comments: 8
Patch Set 5 : Address comments #
Total comments: 2
Patch Set 6 : Rebase #Patch Set 7 : Convert int constants to TimeDelta #
Messages
Total messages: 33 (23 generated)
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tnagel@chromium.org changed reviewers: + emaxx@chromium.org
Hi Maksim, could you please take a look? Thank you, Thiemo
Hi Maksim, could you please take a look? Thank you, Thiemo
Description was changed from ========== Chromad: Refresh policy every 90 minutes BUG=677304 ========== to ========== Chromad: Refresh policy every 90 minutes Applies both to user and device policy. BUG=677304 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm not sure I understood all the potential corner cases in the code, but please see below for my suggestions. It's probably useful to start with the comment regarding ActiveDirectoryPolicyManager::RunScheduledRefresh and CancelableClosure. https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:26: // Minimum delay when scheduling a policy refresh task (to rule out the Have I understood correctly that this constant is mainly for preventing task "storm" in case of a future bug introduced into the code? I'm not sure then that complicating the code with this constant is really helpful against this potential issue. https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:106: refresh_in_progress_ = false; Shouldn't this be done in OnPolicyRefreshed instead? It looks like the assignments to refresh_in_progress_ are not paired up currently (for instance, in case of a user-initiated policy fetch). https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:163: if (now < last_refresh_ + refresh_interval_) { I'm feeling a bit uncomfortable with the possibility to do maths here with a default-initialized TimeTicks last_refresh_. Maybe add a check against is_null into this condition? https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:168: weak_ptr_factory_.GetWeakPtr(), ++task_number_), Maybe do the increment before the call? It's a bit difficult to see it here. Typically these long "Bind" expressions contain only passing the parameters, so one may not expect a piece of logic there. https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:173: // There may be multiple tasks in the queue at the same time (e.g. after Hmm... I was going to suggest using base::CancelableClosure (like CloudPolicyRefreshScheduler does), as in principle it allows to omit tracking this task_number. However, I realized then that it's probably unsafe to call CancelableClosure::Cancel/CancelableClosure::Reset from inside the callback itself, as this could probably invalidate weak pointers and destroy this... But if you think that this case is impossible, then using CancelableClosure would simplify the code. https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:180: // Re-schedule in case the intended refresh interval has not yet been reached Why is this really necessary? Shouldn't all the cases described here end up in OnStoreLoaded/OnStoreError, which do the re-scheduling themselves? https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.h:75: int task_number_ = 0; Worth adding a comment for this member. Also, maybe, adding "refresh_" into its title.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Patchset #4 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
As usual, many thanks for your excellent comments. I think the code has greatly improved. Could you please take another look? https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:26: // Minimum delay when scheduling a policy refresh task (to rule out the On 2017/01/24 15:54:26, emaxx wrote: > Have I understood correctly that this constant is mainly for preventing task > "storm" in case of a future bug introduced into the code? I'm not sure then that > complicating the code with this constant is really helpful against this > potential issue. I also used it for re-scheduling when refresh_in_progress_. Renamed it. https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:106: refresh_in_progress_ = false; On 2017/01/24 15:54:26, emaxx wrote: > Shouldn't this be done in OnPolicyRefreshed instead? It looks like the > assignments to refresh_in_progress_ are not paired up currently (for instance, > in case of a user-initiated policy fetch). I think it's paired correctly for user-initiated fetch, but not in case store->Load() is triggered, as it is e.g. in Init(). Thus I'm foregoing the minor benefit of having store->Load() inside the protected section for greater reliability and change the code to release the lock in OnPolicyRefreshed(). https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:163: if (now < last_refresh_ + refresh_interval_) { On 2017/01/24 15:54:26, emaxx wrote: > I'm feeling a bit uncomfortable with the possibility to do maths here with a > default-initialized TimeTicks last_refresh_. > Maybe add a check against is_null into this condition? This calculation is written with default-initialized (0) TimeTicks in mind. Shall I make it explicit by adding a comment? https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:168: weak_ptr_factory_.GetWeakPtr(), ++task_number_), On 2017/01/24 15:54:26, emaxx wrote: > Maybe do the increment before the call? It's a bit difficult to see it here. > Typically these long "Bind" expressions contain only passing the parameters, so > one may not expect a piece of logic there. Thanks, that's obsolete now. https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:173: // There may be multiple tasks in the queue at the same time (e.g. after > But if you think that this case is impossible, then using CancelableClosure > would simplify the code. Seems like a good idea! Posting ScheduleRefresh() to the loop should avoid self-cancellation. https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:180: // Re-schedule in case the intended refresh interval has not yet been reached On 2017/01/24 15:54:26, emaxx wrote: > Why is this really necessary? Shouldn't all the cases described here end up in > OnStoreLoaded/OnStoreError, which do the re-scheduling themselves? Seems a valid argument. ;) https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2652653007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.h:75: int task_number_ = 0; On 2017/01/24 15:54:26, emaxx wrote: > Worth adding a comment for this member. > Also, maybe, adding "refresh_" into its title. Obsolete.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits and a question https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:7: #include <algorithm> nit: not needed? https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:169: // Re-schedule when a refresh is currently in progress (to avoid D-Bus jobs Sorry, I'm still a bit puzzled about why this re-scheduling is necessary. If there is a refresh happening right now, then, eventually, we will end up in OnPolicyRefreshed, that, by calling ScheduleAutomaticRefresh, will anyway forcibly schedule the next refresh in kRefreshIntervalSeconds. So the re-scheduling here shouldn't bring much value - or am I overlooking some case here? https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.h:77: base::TimeTicks startup_ = base::TimeTicks::Now(); nit: const? https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.h:78: base::TimeTicks last_refresh_{}; nit: Can the bracket initialization be omitted here?
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! Could you please take another look? https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:7: #include <algorithm> On 2017/01/26 20:54:02, emaxx wrote: > nit: not needed? Done. https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:169: // Re-schedule when a refresh is currently in progress (to avoid D-Bus jobs On 2017/01/26 20:54:02, emaxx wrote: > Sorry, I'm still a bit puzzled about why this re-scheduling is necessary. > If there is a refresh happening right now, then, eventually, we will end up in > OnPolicyRefreshed, that, by calling ScheduleAutomaticRefresh, will anyway > forcibly schedule the next refresh in kRefreshIntervalSeconds. > So the re-scheduling here shouldn't bring much value - or am I overlooking some > case here? Your reasoning is sound. I wanted to a manual refresh to override the usual refresh interval, but it's not possible in this way. I'm adding a note. https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.h:77: base::TimeTicks startup_ = base::TimeTicks::Now(); On 2017/01/26 20:54:02, emaxx wrote: > nit: const? Done. https://codereview.chromium.org/2652653007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.h:78: base::TimeTicks last_refresh_{}; On 2017/01/26 20:54:02, emaxx wrote: > nit: Can the bracket initialization be omitted here? Sure.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still LGTM https://codereview.chromium.org/2652653007/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2652653007/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:23: constexpr int kRefreshIntervalSeconds = 90 * 60; BTW, a constexpr base::TimeDelta may be used here directly, which would be arguably a bit easier to read. (Sorry for commenting this too late, just remembered about this possibility.)
Thank you! https://codereview.chromium.org/2652653007/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2652653007/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:23: constexpr int kRefreshIntervalSeconds = 90 * 60; On 2017/01/27 13:51:48, emaxx wrote: > BTW, a constexpr base::TimeDelta may be used here directly, which would be > arguably a bit easier to read. > (Sorry for commenting this too late, just remembered about this possibility.) Oh the joys of C++11... :) Done.
The CQ bit was checked by tnagel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2652653007/#ps140001 (title: "Convert int constants to TimeDelta")
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": 140001, "attempt_start_ts": 1485526527667430,
"parent_rev": "a77ff271cfd7bb0602db0d8965657c228c8f591a", "commit_rev":
"5c9d59322119761d3b2b8569345b7cdd5cec6f2a"}
Message was sent while issue was closed.
Description was changed from ========== Chromad: Refresh policy every 90 minutes Applies both to user and device policy. BUG=677304 ========== to ========== Chromad: Refresh policy every 90 minutes Applies both to user and device policy. BUG=677304 Review-Url: https://codereview.chromium.org/2652653007 Cr-Commit-Position: refs/heads/master@{#446665} Committed: https://chromium.googlesource.com/chromium/src/+/5c9d59322119761d3b2b8569345b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/5c9d59322119761d3b2b8569345b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
