Delayed write to disk if multiple updates is scheduled to persist http_server_properties to disk |
||
Issue descriptionIn a testing for cronet with AGSA on Android, it is identified the data http_server_properties (in memory) is seldom or never persist to disk due to multiple update request within the timer period. In the test, we found from the app start time 12-01 11:51:51.720 until it's killed (around 11:59:19.628), altogether 8 minutes: 38 update has been scheduled with no write to disk eventually triggered. After tracking down the logs, it's discovered that most recent scheduled update will set a timer for 1 min while any new update will reset timer. This might help explain why the race_cert_verification work is not working as expected as the server configs are rarely written to the disk in some cases: user start with few server configs cached and swipe away the app to kill it immediately after active usage.
,
Dec 2 2016
This is a great find! I was looking at this a while back ( Issue 507389 ) but never got down to the root cause. Really glad that the scheduling logic will be improved for mobile.
,
Dec 2 2016
,
Dec 2 2016
Great find zhongyi@. Last Friday when I meet zhongyi@, I had mentioned that we should verify the data is really being persisted to disk or not(on mobile). We didn't want to hit the disk for every change. We used to persist every 5 secs (long ago), but that uses the battery (on Mac??). Ryan H. would have better ideas (highly recommend talking with either Ryan and/or Bence). There could be couple of alternatives, persist at least once every minute (don't do the extension logic, we did this because we were persisting every 5 secs). Another alternative be having a limit. Make sure the data is persisted every 3 minutes (don't cancel more than 3 times).
,
Dec 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5d910ead935f4928dd8a41042b2cbba498ca8fc4 commit 5d910ead935f4928dd8a41042b2cbba498ca8fc4 Author: zhongyi <zhongyi@chromium.org> Date: Fri Dec 09 02:59:18 2016 Change SequencedTaskRunner to SingleThreadTaskRunner in HttpServerPropertiesManager BUG= 670519 Review-Url: https://codereview.chromium.org/2562843002 Cr-Commit-Position: refs/heads/master@{#437445} [modify] https://crrev.com/5d910ead935f4928dd8a41042b2cbba498ca8fc4/net/http/http_server_properties_manager.cc [modify] https://crrev.com/5d910ead935f4928dd8a41042b2cbba498ca8fc4/net/http/http_server_properties_manager.h
,
Dec 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/65aa8f6576dce9b163c7c53fe5aecc9a1b44c00b commit 65aa8f6576dce9b163c7c53fe5aecc9a1b44c00b Author: zhongyi <zhongyi@chromium.org> Date: Tue Dec 13 01:41:03 2016 Change the HttpServerPropertiesManager::net_task_runner_ to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG= 670519 Review-Url: https://codereview.chromium.org/2563273002 Cr-Commit-Position: refs/heads/master@{#437980} [modify] https://crrev.com/65aa8f6576dce9b163c7c53fe5aecc9a1b44c00b/net/http/http_server_properties_manager.cc [modify] https://crrev.com/65aa8f6576dce9b163c7c53fe5aecc9a1b44c00b/net/http/http_server_properties_manager_unittest.cc
,
Dec 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/894658381ab6a524afa5ab08c780f8b4e4f3cb01 commit 894658381ab6a524afa5ab08c780f8b4e4f3cb01 Author: zhongyi <zhongyi@chromium.org> Date: Sat Dec 17 00:08:01 2016 Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner. HttpServerPropertiesManager used to get pref_task_runner by calling base::ThreadTaskRunnerHandle::Get() itself, now changes to take the task runner passed in. No functional change. There are two SingleThreadTaskRunners in HttpServerPropertiesManager: pref_task_runner: which is used to post cache update on pref thread, and should be registered with pref_cache_update_timer_; network_task_runner: which is used to post pref update on network thread, and should be registered with network_prefs_update_timer. BUG= 670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2567893002 Cr-Commit-Position: refs/heads/master@{#439242} [modify] https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01/chrome/browser/net/http_server_properties_manager_factory.cc [modify] https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01/components/cronet/android/cronet_url_request_context_adapter.cc [modify] https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01/ios/chrome/browser/net/http_server_properties_manager_factory.cc [modify] https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01/net/http/http_server_properties_manager.cc [modify] https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01/net/http/http_server_properties_manager.h [modify] https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01/net/http/http_server_properties_manager_unittest.cc
,
Dec 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/56fd0975bd8aded8e004fd4d52fc80e2fd51898c commit 56fd0975bd8aded8e004fd4d52fc80e2fd51898c Author: zhongyi <zhongyi@chromium.org> Date: Sun Dec 18 17:17:00 2016 Change HttpServerPropertiesManager::pref_task_runner_ to use TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that used to ExpectPrefUpdate() will need to call pref_task_runner_->FastForwardUntilNoTasksRemain() so that all the pref tasks complete running. Note running the pref(net)_task_runner might result in new tasks posted to net(pref)_task_runner. BUG= 670519 Review-Url: https://codereview.chromium.org/2577863002 Cr-Commit-Position: refs/heads/master@{#439374} [modify] https://crrev.com/56fd0975bd8aded8e004fd4d52fc80e2fd51898c/net/http/http_server_properties_manager_unittest.cc
,
Dec 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3ba27dc9d493c491e6172b6537d44617a22ea81a commit 3ba27dc9d493c491e6172b6537d44617a22ea81a Author: zhongyi <zhongyi@chromium.org> Date: Mon Dec 19 02:58:59 2016 Change http_server_properties_manager to always persist data to disk after 60s from the receiving update request. All the update requests received during the gap will bundled to the first request. BUG= 670519 Review-Url: https://codereview.chromium.org/2554723003 Cr-Commit-Position: refs/heads/master@{#439395} [modify] https://crrev.com/3ba27dc9d493c491e6172b6537d44617a22ea81a/net/http/http_server_properties_manager.cc [modify] https://crrev.com/3ba27dc9d493c491e6172b6537d44617a22ea81a/net/http/http_server_properties_manager_unittest.cc
,
Dec 19 2016
This bug is now fixed. We'll also need to fix the logic to persist data from pref to memory though.
,
Dec 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3eb8769e7bf92c387b60860e85470b49b4a3fd80 commit 3eb8769e7bf92c387b60860e85470b49b4a3fd80 Author: zhongyi <zhongyi@chromium.org> Date: Mon Dec 19 20:15:48 2016 Enforce check on how many pref updates and cache updates are called in http_server_properties_manager_unittests BUG= 670519 Review-Url: https://codereview.chromium.org/2585823003 Cr-Commit-Position: refs/heads/master@{#439535} [modify] https://crrev.com/3eb8769e7bf92c387b60860e85470b49b4a3fd80/net/http/http_server_properties_manager_unittest.cc
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e985ef190a5a32b173b7b867dfd04607a5006679 commit e985ef190a5a32b173b7b867dfd04607a5006679 Author: zhongyi <zhongyi@chromium.org> Date: Wed Dec 21 06:07:44 2016 Change http_server_properties_manager to always persist data to memory after 1s from the receiving update request. All the update requests received during the gap will bundled to the first request. BUG= 670519 Review-Url: https://codereview.chromium.org/2587303002 Cr-Commit-Position: refs/heads/master@{#440035} [modify] https://crrev.com/e985ef190a5a32b173b7b867dfd04607a5006679/net/http/http_server_properties_manager.cc [modify] https://crrev.com/e985ef190a5a32b173b7b867dfd04607a5006679/net/http/http_server_properties_manager_unittest.cc
,
Dec 21 2016
I am going to close this issue since we have fixed the bug in both directions: from disk to memory and from memory to disk. I have manually patched the fix from memory to disk on my local and tested on Android, it worked.
,
Feb 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2f8faeaffdf9e93caaf0dab7e99d733e3d6278d3 commit 2f8faeaffdf9e93caaf0dab7e99d733e3d6278d3 Author: zhongyi <zhongyi@chromium.org> Date: Thu Feb 23 22:04:17 2017 Add a histogram to track how many server configs has been persisted into disk on Android. BUG= 670519 Review-Url: https://codereview.chromium.org/2711803003 Cr-Commit-Position: refs/heads/master@{#452647} [modify] https://crrev.com/2f8faeaffdf9e93caaf0dab7e99d733e3d6278d3/net/http/http_server_properties_manager.cc [modify] https://crrev.com/2f8faeaffdf9e93caaf0dab7e99d733e3d6278d3/tools/metrics/histograms/histograms.xml |
||
►
Sign in to add a comment |
||
Comment 1 by zhongyi@chromium.org
, Dec 2 2016Status: Assigned (was: Untriaged)
The code which triggers the cancellation of the scheduled update due to new event: // Time to wait before starting an update the preferences from the // http_server_properties_impl_ cache. Scheduling another update during this // period will reset the timer. const int64_t kUpdatePrefsDelayMs = 60000; void HttpServerPropertiesManager::ScheduleUpdatePrefsOnNetworkThread( Location location) { DCHECK(network_task_runner_->RunsTasksOnCurrentThread()); // Cancel pending updates, if any. network_prefs_update_timer_->Stop(); // Where we stop the timer. StartPrefsUpdateTimerOnNetworkThread( base::TimeDelta::FromMilliseconds(kUpdatePrefsDelayMs)); // TODO(rtenneti): Delete the following histogram after collecting some data. UMA_HISTOGRAM_ENUMERATION("Net.HttpServerProperties.UpdatePrefs", location, HttpServerPropertiesManager::NUM_LOCATIONS); }