Change http_server_properties_manager to always persist data to memory after 1s

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
Committed: https://crrev.com/e985ef190a5a32b173b7b867dfd04607a5006679
Cr-Commit-Position: refs/heads/master@{#440035}

[Zhongyi Shi, 2016-12-20 06:34:04]

zhongyi@chromium.org changed reviewers:
+ rch@chromium.org, xunjieli@chromium.org

[Zhongyi Shi, 2016-12-20 06:34:05]

The follow-up CL to fix persisting data to memory. PTAL!

[xunjieli, 2016-12-20 14:00:27]

Cherie, there's one thing I don't understand about the system.

There looks like to me a circular update from cache to pref, then from pref to
cache, then back to pref. In reality we only need to load from pref to cache
once (during initializing HSPM) and persist to pref every 60s afterwards.

Could you be so kind to check one thing for me? If there is an update to persist
cache to pref, will it trigger an update from pref to cache?  The latter is not
needed and will only waste CPU cycles.

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager.cc:28: const int64_t kUpdateCacheDelayMs
= 1000;
nit: The delay is 1s. Could you edit the CL description?

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager_unittest.cc:1214: ExpectCacheUpdate();
I guess you are doing a follow-up to make sure that there are 3 scheduled tasks
and 2 actual updates? (just like you did for pref?)

[Zhongyi Shi, 2016-12-20 20:00:50]

Description was changed from

==========
Change http_server_properties_manager to always persist data to memory after 60s
from the receiving update request. All the update requests received during the
gap will bundled to the first request.

BUG=670519
==========

to

==========
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
==========

[Zhongyi Shi, 2016-12-20 20:03:39]

Hi Helen,

Thanks for raising the questions. Quick answer here:
Generally, there are two threads HttpServerPropertiesManager involves: network
thread and pref(disk) thread. It will need to persist data between the two
threads. In either direction, both threads will be involved, as the manager will
need to read data on the thread where the data source lives, and pass over the
data by posting task to the other thread where it want to update the data. In
some cases, if we detect data corruption, we’ll issue a reverse direction
update.

I also have a notes I used when investigating the bug: https://goo.gl/e6Kde2
Hope this will answer your question and help the review.

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager.cc:28: const int64_t kUpdateCacheDelayMs
= 1000;
On 2016/12/20 14:00:27, xunjieli wrote:
> nit: The delay is 1s. Could you edit the CL description?

Done.

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager_unittest.cc:1214: ExpectCacheUpdate();
On 2016/12/20 14:00:27, xunjieli wrote:
> I guess you are doing a follow-up to make sure that there are 3 scheduled
tasks
> and 2 actual updates? (just like you did for pref?)

If you take a close look on the previous CL, we check the number of calls of
ScheduleUpdatePrefsOnNetworkThread vs UpdatePrefsFromCacheOnNetworkThread.
That's because we invoke ScheduleUpdatePrefsOnNetworkThread indirectly by
calling SetSupportsSpdy. 

In this CL, we call ScheduleUpdateCacheOnPrefThread directly 3 times, the reason
we don't invoke this method indirectly is because it involves pref_delegate.
Note in the test the ExpectCacheUpdate() is invoked twice. I think it's fine to
leave as is. It's pretty clear that we schedule 3 times but only update twice.

[Ryan Hamilton, 2016-12-20 20:38:38]

The code change looks good, but I'm not sure I understand the answer to Helen's
question. If we update one side, we schedule a task to update the other side.
When that fires, do we schedule a task to update the first side again, lather
rinse repeat?

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager.cc:28: const int64_t kUpdateCacheDelayMs
= 1000;
On 2016/12/20 20:03:38, Zhongyi Shi wrote:
> On 2016/12/20 14:00:27, xunjieli wrote:
> > nit: The delay is 1s. Could you edit the CL description?
> 
> Done.

Confirming: we update net from prefs once a second, and prefs from net once a
minute?

[Zhongyi Shi, 2016-12-20 20:59:23]

On 2016/12/20 20:38:38, Ryan Hamilton wrote:
> The code change looks good, but I'm not sure I understand the answer to
Helen's
> question. If we update one side, we schedule a task to update the other side.
> When that fires, do we schedule a task to update the first side again, lather
> rinse repeat?

It depends. This happens and I think the only case is as follows: if you want to
update data from pref to memory, you'll start with reading data on pref thread,
then post a task to network thread with data passed over (it might also
discovered detected_corrupted_prefs, but detected_corrupted_prefs will be passed
to network thread to handle). Then the network thread will update data in
memory, and if it found detected_corrupted_prefs, it'll
ScheduleUpdatePrefsOnNetworkThread, which means it will now read the data on
network thread, and then post task to pref thread. Pref thread will update the
data in disk, but that's the end. There won't be any new updates to net thread
since the update is coming from the net thread.

> Confirming: we update net from prefs once a second, and prefs from net once a
> minute?
Only if we have received update request. If we never receive any update request,
we won't update. Once the update is scheduled, the delay for pref update is 60s,
and the memory is 1s. Any new update scheduled within the time window will be
no-op, automatically bundled to the one that previously successfully scheduled.

[Ryan Hamilton, 2016-12-20 21:39:07]

On 2016/12/20 20:59:23, Zhongyi Shi wrote:
> On 2016/12/20 20:38:38, Ryan Hamilton wrote:
> > The code change looks good, but I'm not sure I understand the answer to
> Helen's
> > question. If we update one side, we schedule a task to update the other
side.
> > When that fires, do we schedule a task to update the first side again,
lather
> > rinse repeat?
> 
> It depends. This happens and I think the only case is as follows: if you want
to
> update data from pref to memory, you'll start with reading data on pref
thread,
> then post a task to network thread with data passed over (it might also
> discovered detected_corrupted_prefs, but detected_corrupted_prefs will be
passed
> to network thread to handle). Then the network thread will update data in
> memory, and if it found detected_corrupted_prefs, it'll
> ScheduleUpdatePrefsOnNetworkThread, which means it will now read the data on
> network thread, and then post task to pref thread. Pref thread will update the
> data in disk, but that's the end. There won't be any new updates to net thread
> since the update is coming from the net thread.
> 
> > Confirming: we update net from prefs once a second, and prefs from net once
a
> > minute?
> Only if we have received update request. If we never receive any update
request,
> we won't update. Once the update is scheduled, the delay for pref update is
60s,
> and the memory is 1s. Any new update scheduled within the time window will be
> no-op, automatically bundled to the one that previously successfully
scheduled.

Ok, great. Thanks for the walkthrough. LGTM. Please wait for Helen's approval,
too.

[xunjieli, 2016-12-20 22:31:22]

On 2016/12/20 21:39:07, Ryan Hamilton wrote:
> On 2016/12/20 20:59:23, Zhongyi Shi wrote:
> > On 2016/12/20 20:38:38, Ryan Hamilton wrote:
> > > The code change looks good, but I'm not sure I understand the answer to
> > Helen's
> > > question. If we update one side, we schedule a task to update the other
> side.
> > > When that fires, do we schedule a task to update the first side again,
> lather
> > > rinse repeat?
> > 
> > It depends. This happens and I think the only case is as follows: if you
want
> to
> > update data from pref to memory, you'll start with reading data on pref
> thread,
> > then post a task to network thread with data passed over (it might also
> > discovered detected_corrupted_prefs, but detected_corrupted_prefs will be
> passed
> > to network thread to handle). Then the network thread will update data in
> > memory, and if it found detected_corrupted_prefs, it'll
> > ScheduleUpdatePrefsOnNetworkThread, which means it will now read the data on
> > network thread, and then post task to pref thread. Pref thread will update
the
> > data in disk, but that's the end. There won't be any new updates to net
thread
> > since the update is coming from the net thread.
> > 
> > > Confirming: we update net from prefs once a second, and prefs from net
once
> a
> > > minute?
> > Only if we have received update request. If we never receive any update
> request,
> > we won't update. Once the update is scheduled, the delay for pref update is
> 60s,
> > and the memory is 1s. Any new update scheduled within the time window will
be
> > no-op, automatically bundled to the one that previously successfully
> scheduled.
> 
> Ok, great. Thanks for the walkthrough. LGTM. Please wait for Helen's approval,
> too.

Sorry for any delay. I am still trying to understand the code. I will get back
to you by tomorrow morning.

[xunjieli, 2016-12-21 02:13:09]

lgtm. Thanks for the patience! You are right. There won't be a non-ending
circular update. There are two magic booleans which I missed in my first reading
of the code. As you pointed out, one of them is |detected_corrupted_prefs|,
which prevents a prefs-to-cache update from going back to prefs. The second
boolean is |setting_pref|, which prevents a cache-to-prefs update from going
back to cache (OnHttpServerPropertiesChange is called synchronously during
pref_service->Set(), so |setting_pref| will be true and cache update is
skipped). |setting_pref| is interesting.  But that is probably not a concern.
Since even if the pref change notification is posted asynchronously,
|detected_corrupted_prefs| should be able to end the cycle.

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2587303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager_unittest.cc:1214: ExpectCacheUpdate();
On 2016/12/20 20:03:38, Zhongyi Shi wrote:
> On 2016/12/20 14:00:27, xunjieli wrote:
> > I guess you are doing a follow-up to make sure that there are 3 scheduled
> tasks
> > and 2 actual updates? (just like you did for pref?)
> 
> If you take a close look on the previous CL, we check the number of calls of
> ScheduleUpdatePrefsOnNetworkThread vs UpdatePrefsFromCacheOnNetworkThread.
> That's because we invoke ScheduleUpdatePrefsOnNetworkThread indirectly by
> calling SetSupportsSpdy. 
> 
> In this CL, we call ScheduleUpdateCacheOnPrefThread directly 3 times, the
reason
> we don't invoke this method indirectly is because it involves pref_delegate.
> Note in the test the ExpectCacheUpdate() is invoked twice. I think it's fine
to
> leave as is. It's pretty clear that we schedule 3 times but only update twice.


Acknowledged.

[Zhongyi Shi, 2016-12-21 04:57:39]

Thanks a lot for helping the review and getting to bottom of the code, Helen and
Ryan!  Landing now, this will be the last CL to fix the linked bug.

[Zhongyi Shi, 2016-12-21 04:57:50]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2016-12-21 04:58:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 06:07:47]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1482296270970260, "parent_rev":
"846222634bcd3f1fd57e9e7bf3140ff5e7131cef", "commit_rev":
"05830d089bf0fc913c61417ad9f6c5552c713020"}

[commit-bot: I haz the power, 2016-12-21 06:08:25]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-12-21 06:08:26]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-21 06:13:04]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/e985ef190a5a32b173b7b867dfd04607a5006679
Cr-Commit-Position: refs/heads/master@{#440035}
==========

[commit-bot: I haz the power, 2016-12-21 06:13:04]

Patchset 1 (id:??) landed as
https://crrev.com/e985ef190a5a32b173b7b867dfd04607a5006679
Cr-Commit-Position: refs/heads/master@{#440035}