349809 - add random intervals to update requests

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

(another requirement from launch engineering)

If anti-phishing is on, we make an update request for local lists when Firefox starts up and then once every hour.  Rather than having a rigid time, we should add a small random interval (e.g., 60 min + (rand() * 5 min)).  This avoids the case of everyone hitting the server at exactly the same time.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't know if this proposal actually solves that problem, but sure, we'll take a small timer fix here :)

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Instead of adding up to five minutes of randomness, it seems like it'd sense to pick a random number between 0 and 59 each time a user starts up and then start updating on that minute every hour.  Then we'll be distributing loads evenly across the minutes in the hour, regardless of the distribution of starts across them.

So, after checking for the first time on startup, check for the second time in
30 + (rand() * 59) minutes, which distributes the second check between 30 and 89 minutes (an average of one hour) away.  Then commence checking every 60 minutes.

Comment 3

[Tony Chang (Google)]

(In reply to comment #2)
> Instead of adding up to five minutes of randomness, it seems like it'd sense to
> pick a random number between 0 and 59 each time a user starts up and then start
> updating on that minute every hour.  Then we'll be distributing loads evenly
> across the minutes in the hour, regardless of the distribution of starts across
> them.
> 
> So, after checking for the first time on startup, check for the second time in
> 30 + (rand() * 59) minutes, which distributes the second check between 30 and
> 89 minutes (an average of one hour) away.  Then commence checking every 60
> minutes.

This sounds better and shouldn't be riskier to implement.  I'll go with this.

Comment 4

[Tony Chang (Google)]

Err, the blocking flag accidentally got toggled off.

Comment 5

[Tony Chang (Google)]

Attachment: v1: add random interval to table update requests

- going with myk's suggestion

Comment 6

[Niels Provos]

Comment on attachment 235270 [details] [diff] [review]
v1: add random interval to table update requests

I am a little bit confused about the use of repeating alarms here.  at the moment, initialUpdateCheck is called for each update and it installs a repeating alarm.  wouldn't it be cleaner to make that a non repeating alarm or call initialUpdateCheck only once and then go back to the CheckForUpdates function?

Comment 7

[Tony Chang (Google)]

(In reply to comment #6)
> (From update of attachment 235270 [details] [diff] [review] [edit])
> I am a little bit confused about the use of repeating alarms here.  at the
> moment, initialUpdateCheck is called for each update and it installs a
> repeating alarm.  wouldn't it be cleaner to make that a non repeating alarm or
> call initialUpdateCheck only once and then go back to the CheckForUpdates
> function?

It does your second suggestion of calling initialUpdateCheck only once then going back to checkForUpdates.  Perhaps the comments are confusing?

Comment 8

[Niels Provos]

Comment on attachment 235270 [details] [diff] [review]
v1: add random interval to table update requests

looks fine.  i must have misread earlier.

Comment 9

[Tony Chang (Google)]

Attachment: v2: switch to 30 min update interval

Offline, Niels said it should be 30 minutes.  Same as before except first scheduled check is between 15-45 min, then every 30 min after that.

Comment 10

[Niels Provos]

Comment on attachment 235464 [details] [diff] [review]
v2: switch to 30 min update interval

you need to adjust some of the comments that talk about 60 minute intervals.

Comment 11

[Tony Chang (Google)]

Attachment: v3: fix comments

Comment 12

[Tony Chang (Google)]

on trunk

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 235475 [details] [diff] [review]
v3: fix comments

a=beltzner on behalf of 181drivers

Comment 14

[Tony Chang (Google)]

on branch