664792 - Tune sync intervals according to user behaviour

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

This is a follow-up from bug 600429 to complete the polling part of Instant Sync. The idea is that

* on "back" observer with threshold (suggested 5 mins) we
  - sync
  - change sync interval to non-idle one 
* on "idle" observer with threshold (5 mins) we
  - change sync interval to idle one 
* after a sync while non-idle we
  - decrease sync interval if items were downloaded
  - increas sync interval if no items were downloaded

Comment 1

[Marina Samuel [:emtwo]]

Attachment: Part 1 (WIP): remove heartbeat triggered sync

Comment 2

[Philipp von Weitershausen [:philikon]]

Comment on attachment 539872 [details] [diff] [review]
Part 1 (WIP): remove heartbeat triggered sync

carrying over the r+ from bug 600429

Comment 3

[Marina Samuel [:emtwo]]

Attachment: Part 2 (WIP): creating SyncScheduler component & tests

* passed unit & tps tests

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 540125 [details] [diff] [review]
Part 2 (WIP): creating SyncScheduler component & tests

>+function run_test() {
>+  initTestLogging("Trace");
>+
>+  Log4Moz.repository.getLogger("Sync.Service").level = Log4Moz.Level.Trace;
>+
>+  test_updateClientMode();
>+  test_login_rejected_masterpassword_interval();
>+  test_calculateBackoff();
>+  test_scheduleNextSync();
>+  test_handleSyncError();
>+}

It would be nice if you could use the run_next_test + add_test() pattern, makes it easier to add more tests, especially when they turn out to be asynchronous.

>+function test_scheduleNextSync() {
...
>+  // syncInterval is smaller than expectedInterval
>+  // since some time has passed.
>+  do_check_true(syncInterval <= expectedInterval);
>+}

We might want to cancel the timer at the end of this test so we actually end up triggering a sync.


>+[test_policies_scheduling.js]
...
>+[test_syncscheduler_attributes.js]

<bikeshed>
Why the different naming schemes? I suggest folding them all into one test called test_syncscheduler.js :)
</bikeshed>


r=me with those

Comment 5

[Marina Samuel [:emtwo]]

(In reply to comment #4)
> >+function test_scheduleNextSync() {
> ...
> >+  // syncInterval is smaller than expectedInterval
> >+  // since some time has passed.
> >+  do_check_true(syncInterval <= expectedInterval);
> >+}
> 
> We might want to cancel the timer at the end of this test so we actually end
> up triggering a sync.

I'm not sure what you're talking about here.  Do you mean cancelling the _syncTimer? Wouldn't cancelling it *not* trigger a sync?

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to comment #5)
> > We might want to cancel the timer at the end of this test so we actually end
> > up triggering a sync.
> 
> I'm not sure what you're talking about here.  Do you mean cancelling the
> _syncTimer? Wouldn't cancelling it *not* trigger a sync?

Well, yes. It seems like you're just testing the size of the delays, so we wouldn't want to accidentally kick off a sync in the test. Of course, now that you mention it, it might make sense to actually test for that, much like we did in test_score_triggers.js.

Comment 7

[Marina Samuel [:emtwo]]

Attachment: Part 2 (v1): creating SyncScheduler component & tests

* addressed changes from comment 4
* didn't do a wait on sync finish since the intervals being tested in test_scheduleNextSync would cause the test to take a long time to wait for. Cancelled the syncTimer though.

Comment 8

[Marina Samuel [:emtwo]]

Attachment: Part 3 (v1): autoconnect changes

Comment 9

[Marina Samuel [:emtwo]]

(In reply to comment #8)
> Created attachment 540659 [details] [diff] [review] [review]
> Part 3 (WIP): autoconnect changes

* Passed tps & unit tests
* crossweave.jsm has 3 references to autoconnect pref which I didn't remove (not sure if I should be touching that code?

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 540659 [details] [diff] [review]
Part 3 (v1): autoconnect changes

>@@ -1049,55 +1049,50 @@ WeaveSvc.prototype = {
>       Services.logins.removeLogin(login);
>     });
>   },
> 
>   delayedAutoConnect: function delayedAutoConnect(delay) {

Can you add a comment above this method explaining what it does? Something along these lines perhaps:

 /**
  * Automatically start syncing after the given delay (in seconds).
  *
  * Applications can define the `services.sync.autoconnectDelay` preference
  * to have this called automatically during start-up with the pref value as
  * the argument. Alternatively, they can call it themselves to control when
  * Sync should first start to sync.
  */
  
>     if (this._loggedIn)
>       return;

I think you can get rid of this `if` clause. We've redefined "autoconnect" as "autosyncing" which can proceed if we're already logged in.

>   _autoConnect: let (attempts = 0) function _autoConnect() {

We got rid of the attempts stuff below, so we can get rid of the `let` closure here, too

>-    // Something failed, so try again some time later.
>-    let interval = Utils.calculateBackoff(++attempts, 60 * 1000);
>-    this._log.debug("Autoconnect failed: " + (reason || Status.login) +
>-      "; retry in " + Math.ceil(interval / 1000) + " sec.");
>-    Utils.namedTimer(this._autoConnect, interval, this, "_autoTimer");
>+    // Once _autoConnect is called we no longer need _autoTimer.
>+    if (this._autoTimer)
>+      _autoTimer.clear();

Missing `this.` before _autoTimer.clear();


Looks good otherwise, but I'll have mfinkle look over the mobile parts. Some context for mfinkle: we've long ago removed the notion of manual login/logout (as a way to start and stop syncing while still being connected). The 'autoconnect' pref is a relic from those times, it persisted this state across browser launches. This patch gets rid of that and streamlines the startup -> autoconnect -> login -> eventually-sync to startup -> autoconnect -> sync-goddammit-already.

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to comment #9)
> * crossweave.jsm has 3 references to autoconnect pref which I didn't remove
> (not sure if I should be touching that code?

We can do that as a follow-up. It looks pretty harmless, especially if TPS passes. Let's get this bug wrapped up first.

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 540635 [details] [diff] [review]
Part 2 (v1): creating SyncScheduler component & tests

>   sync: function sync() {
>     let dateStr = new Date().toLocaleFormat(LOG_DATE_FORMAT);
>     this._log.info("Starting sync at " + dateStr);
>     this._catch(function () {
>       // Make sure we're logged in.
>       if (this._shouldLogin()) {
>         this._log.debug("In sync: should login.");
>         if (!this.login()) {
>           this._log.debug("Not syncing: login returned false.");
>-          this._clearSyncTriggers();    // No more pending syncs, please.
>-
>-          // Try again later, just as if we threw an error... only without the
>-          // error count.
>-          if (!this._skipScheduledRetry())
>-            this._scheduleAtInterval(MASTER_PASSWORD_LOCKED_RETRY_INTERVAL);
>-
>+          Svc.Obs.notify("weave:service:login:failed");    // No more pending syncs, please.
>           return;

I've just noticed this: a failed login() should already notify "weave:service:login:failed", so no need for that line.

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to comment #12)
> I've just noticed this: a failed login() should already notify
> "weave:service:login:failed", so no need for that line.

Argh, it doesn't if the master password is locked. Exhibit A from Service.login():

      if (!this.verifyLogin()) {
        if (Status.login == MASTER_PASSWORD_LOCKED) {
          // Drat.
          this._log.debug("Login failed: " + Status.login);
          return false;
        }
        // verifyLogin sets the failure states here.
        throw "Login failed: " + Status.login;
      }

Can you get rid of the if (Status.login == MASTER_PASSWORD_LOCKED) clause here, please? We should always throw to ensure that login:failed rather than login:finished is notified. Thanks to Utils.catch, throwing will be equivalent to returning something untruthy, so consumers of Service.login() should still get the same boolean return value.

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 540635 [details] [diff] [review]
Part 2 (v1): creating SyncScheduler component & tests

Bah, I now realize what went wrong. In bug 600429 comment 31 as well as in comment 12 and comment 13 here I was speaking of "weave:service:login:failed" but I meant "weave:service:login:error", which is automatically notified if Service.login() throws.

So, to recap:

* please make Service.login() *always* throw if verifyLogin() returns false
* observe weave:service:login:error and get rid of the custom weave:service:login:failed notification.

Sorry for being confusing, I totally mixed up :failed and :error here. Withdrawing the r+ for now.

Comment 15

[Marina Samuel [:emtwo]]

Attachment: Part 2 (v2): creating SyncScheduler component & tests

Comment 16

[Marina Samuel [:emtwo]]

Attachment: Part 3 (v2): autoconnect changes

Comment 17

[Philipp von Weitershausen [:philikon]]

Comment on attachment 540893 [details] [diff] [review]
Part 3 (v2): autoconnect changes

Thumbs up from me, pending feedback from mfinkle. See bottom of comment 10 for context.

Comment 18

[Marina Samuel [:emtwo]]

Attachment: Part 4 (WIP): sync intervals

* very rough & untested but gives an idea of how I'm approaching the problem

Comment 19

[Philipp von Weitershausen [:philikon]]

Comment on attachment 540861 [details] [diff] [review]
Part 2 (v2): creating SyncScheduler component & tests

Big patch is big... found one more problem:

>+      case "weave:service:sync:finish":
>+        let sync_interval;
>+        this._syncErrors = 0;
>+
>+        if (subject == "clients") {
>+          // Update the client mode because it might change what we sync.
>+          this.updateClientMode();
>+        }

The `if` clause should be happening in "weave:engine:sync:finish", not "weave:service:sync:finish" (see bottom of bug 600429 comment 31.) The idea is that we update the client mode after having sync'ed the Clients engine.

We will need a test for this, but since updateClientMode's behaviour gets changed in Part 4 anyway, you can add the test in Part 4.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 540893 [details] [diff] [review]
Part 3 (v2): autoconnect changes

Mobile part looks good

Comment 21

[Philipp von Weitershausen [:philikon]]

Comment on attachment 540945 [details] [diff] [review]
Part 4 (WIP): sync intervals

This is looking good, don't be intimidated by the amount of comments :)

>-SINGLE_USER_SYNC:                      24 * 60 * 60 * 1000, // 1 day
>+SINGLE_USER_SYNC:                      60 * 60 * 1000, // 1 hour

Let's not change this interval until we understand the impact on our servers. The vast majority of our users are single device users, remember? :)

>     count.newFailed = Utils.arraySub(this.previousFailed, failedInPreviousSync).length;
>     if (count.newFailed) {
>       // Notify observers if records failed to apply. Pass the count object
>       // along so that they can make an informed decision on what to do.
>       Observers.notify("weave:engine:sync:apply-failed", count, this.name);
>     }
>+    Observers.notify("weave:engine:sync:applied", count, this.name);

With both 'weave:engine:sync:apply-failed' and 'weave:engine:sync:applied' we have a bit of redundancy. Let's get rid of 'apply-failed' altogether. This means changing the observer in Service to listen for 'applied' and checking for count.newFailed there (count should be the subject, I think).

>+  // A user is assumed to have a single device upon startup
>+  // and is therefore inactive.
>+  multidevice_active: false,

I think we'll need two flags here:

* one whether the user has multiple devices or not (you can use
  Service.numClients > 1 here)

* one to indicate whether the user is idle or active (I suggest turning
  the boolean logic around and calling it 'idle' rather than 'active'
  since that's what our API talks about).

Because these things get set independently at different times (idle/back observer changes the idle state and updateClientMode changes the single/multi-device state.) See below.


Thanks for commenting the flag, btw. Can you please also document how initial setting are expected to get changed? E.g.:

  This will change upon the first sync right after startup, when we
  sync the Clients engine and call updateClientMode().

Lastly, a nit: please stick to camelCase for attribute names.

>   // nextSync is in milliseconds, but prefs can't hold that much
>   get nextSync() Svc.Prefs.get("nextSync", 0) * 1000,
>   set nextSync(value) Svc.Prefs.set("nextSync", Math.floor(value / 1000)),
> 
>-  get syncInterval() {
>-    // If we have a partial download, sync sooner if we're not mobile
>-    if (Status.partial && Clients.clientType != "mobile")
>-      return PARTIAL_DATA_SYNC;
>-    return Svc.Prefs.get("syncInterval", MULTI_MOBILE_SYNC);
>-  },
>+  get syncInterval() Svc.Prefs.get("syncInterval", SINGLE_USER_SYNC),
>   set syncInterval(value) Svc.Prefs.set("syncInterval", value),
> 
>   get syncThreshold() Svc.Prefs.get("syncThreshold", SINGLE_USER_THRESHOLD),
>   set syncThreshold(value) Svc.Prefs.set("syncThreshold", value),

As discussed on the phone the other day, I don't think we need to keep 'syncInterval' as a pref anymore. The 'nextSync' pref also seems useless since we'll sync right after startup now, so why persist it. 'syncThreshold' gets set in updateClientMode right during the first sync, so all we need is a sensible default here:

  nextSync: 0,
  syncInterval: SINGLE_USER_SYNC,
  syncThreshold: SINGLE_USER_THRESHOLD,

>+  numAppliedThisSync: 0,
>+
...
>+      case "weave:engine:sync:applied":
>+        this._log.trace("Engine " + data + " applied items.");
>+        this.numAppliedThisSync += subject.applied.length;

subject.applied is already the count, so no need for '.length'.

>+  adjustSyncInterval: function adjustSyncInterval() {
>+    if (!multidevice_active) {
>+      return;
>+    }

Forgot 'this.' here

>+    if (this.numAppliedThisSync) {
>+      this.numAppliedThisSync = 0;
>+      this.syncInterval = MULTI_DEVICE_IMMEDIATE_SYNC;
>+    } else {
>+      this.syncInterval = MULTI_DEVICE_ACTIVE_SYNC;
>+    }

So you're just treating this.numAppliedThisSync as a boolean, so why aggregate the number at all? I suggest a boolean property (e.g. 'haveIncomingItems') that gets set to true in the "weave:engine:sync:applied" observer, *if* subject.applied was non-zero, and then it gets reset here.

Also, adjustSyncInterval doesn't treat idle v. active right now. The above logic is only necessary when we're in active mode. When we're idle we should obviously use MULTI_DEVICE_IDLE_SYNC all the way through. Separating the multidevice_active flag into two will help here. With those you will probably want to bail out if (Weave.Service.numClients <= 1 || this.idle).

>+      case "idle":
>+        this.syncInterval = MULTI_DEVICE_IDLE_SYNC;
>+        this.multidevice_active = false;
>+        break;
>+      case "back":
>+        Utils.nextTick(this.sync, this);

I think you meant (Service.sync, Service) here :)

>+        this.syncInterval = MULTI_DEVICE_ACTIVE_SYNC;
>+        this.multidevice_active = true;
>+        break;

...

>   updateClientMode: function updateClientMode() {
>     // Nothing to do if it's the same amount
>-    let {numClients, hasMobile} = Clients.stats;
>+    let {numClients} = Clients.stats;
>     if (Weave.Service.numClients == numClients)
>       return;

Hmm, this `if` clause is a problem, because Service.numClients is persisted as a pref. This means that the rest of updateClientMode is only ever called if the number of clients change. I propose to get rid of the 'numClients' preference, set Service.numClients = 0 by default, so that the rest of SyncScheduler.updateClientMode is guaranteed to run at least once on the first sync after startup.

>     this._log.debug("Client count: " + Weave.Service.numClients + " -> " + numClients);
>     Weave.Service.numClients = numClients;
> 
>-    if (numClients == 1) {
>+    // There were multiple clients, some were removed, now there's 1.
>+    if (numClients == 1 && this.syncInterval != SINGLE_USER_SYNC) {
>       this.syncInterval = SINGLE_USER_SYNC;
>       this.syncThreshold = SINGLE_USER_THRESHOLD;
>+      this.multidevice_active = false;
>+      Svc.Idle.removeIdleObserver(this, IDLE_TIME);
>     }
>-    else {
>-      this.syncInterval = hasMobile ? MULTI_MOBILE_SYNC : MULTI_DESKTOP_SYNC;
>+    // There was 1 client, some were added, now there's multiple.
>+    else if (numClients > 1 & this.syncInterval == SINGLE_USER_SYNC) {

Missing another & here.

>+      this.syncInterval = MULTI_DEVICE_ACTIVE_SYNC;
>       this.syncThreshold = MULTI_DEVICE_THRESHOLD;
>+      this.multidevice_active = true;
>+      Svc.Idle.addIdleObserver(this, IDLE_TIME);
>     }

Why are you setting this.multidevice_active = true here? How does updateClientMode know that we're not idle? (The answer is of course that it doesn't... see my suggestion for splitting up the multidevice_active flag above.)

Another problem is that updateClientMode may be called multiple times during the lifetime of a browser session, e.g. because the user adds or removes devices from the account. Each time we'd adding ourselves as an idle observer. That's not good :) I don't think there would be much harm in *always* being registered as an idle observer (e.g. register it in SyncScheduler.init()), we then just have to gate the syncInterval changes in the idle/back observers with a check for Service.numClients > 1.


>+function test() {
>+/*
>+  // User not interacting wih browser so no idle time detected.
>+  do_check_eq(Svc.Idle.idleTime, 0);

This is a tricky thing to test for in xpcshell and might easily lead to random test failures on headless tinderbox machines. It's also not really necessary to test for this.

Comment 22

[Marina Samuel [:emtwo]]

Attachment: Part 2 (v3): creating SyncScheduler component & tests

* addressing comment 19

Comment 23

[Philipp von Weitershausen [:philikon]]

Comment on attachment 541413 [details] [diff] [review]
Part 2 (v3): creating SyncScheduler component & tests

>+  init: function init() {
>+    Svc.Obs.add("weave:engine:score:updated", this);
>+    Svc.Obs.add("network:offline-status-changed", this);
>+    Svc.Obs.add("weave:service:sync:start", this);
>+    Svc.Obs.add("weave:service:sync:finish", this);
>+    Svc.Obs.add("weave:engine:sync:finish", this);
>+    Svc.Obs.add("weave:service:login:error", this);
>+    Svc.Obs.add("weave:engine:login:finish", this);

Ehm, just noticed this since you changed the line below: This should be weave:service:login:finish... (it's not super terrible since Part 3 removes that line again, but still...)

Comment 24

[Marina Samuel [:emtwo]]

(In reply to comment #23)
> Comment on attachment 541413 [details] [diff] [review] [review]
> Part 2 (v3): creating SyncScheduler component & tests

> Ehm, just noticed this since you changed the line below: This should be
> weave:service:login:finish... (it's not super terrible since Part 3 removes
> that line again, but still...)

heh, bit of mix up between weave:service & weave:engine, not sure how I missed those. Hopefully won't happen again.

Comment 25

[Marina Samuel [:emtwo]]

(In reply to comment #21)
> 
> >+      this.syncInterval = MULTI_DEVICE_ACTIVE_SYNC;
> >       this.syncThreshold = MULTI_DEVICE_THRESHOLD;
> >+      this.multidevice_active = true;
> >+      Svc.Idle.addIdleObserver(this, IDLE_TIME);
> >     }
> 
> Why are you setting this.multidevice_active = true here? How does
> updateClientMode know that we're not idle? (The answer is of course that it
> doesn't... see my suggestion for splitting up the multidevice_active flag
> above.)
> 
> Another problem is that updateClientMode may be called multiple times during
> the lifetime of a browser session, e.g. because the user adds or removes
> devices from the account. Each time we'd adding ourselves as an idle
> observer. That's not good :) I don't think there would be much harm in
> *always* being registered as an idle observer (e.g. register it in
> SyncScheduler.init()), we then just have to gate the syncInterval changes in
> the idle/back observers with a check for Service.numClients > 1.


The point of this line: 

>+    else if (numClients > 1 && this.syncInterval == SINGLE_USER_SYNC) {

is so that adding an idle observer *only* happens once - when you switch from single device to multi-device. So adding ourselves as an idle observer multiple times does not actually happen. For every single addition there is a single removal. So the idea here is that *as soon* as we make the switch from single device to multi device we must assume we are either active or idle. So I think the question is, which one is a better option as the default upon switching from single- to multi-device.  I think even if we are always registered as an idle obserever, we still have to choose between syncInterval = MULTI_DEVICE_ACTIVE_SYNC and MULTI_DEVICE_IDLE_SYNC upon switching to multi-device.

Comment 26

[Philipp von Weitershausen [:philikon]]

(In reply to comment #25)
> The point of this line: 
> 
> >+    else if (numClients > 1 && this.syncInterval == SINGLE_USER_SYNC) {
> 
> is so that adding an idle observer *only* happens once - when you switch
> from single device to multi-device. So adding ourselves as an idle observer
> multiple times does not actually happen. For every single addition there is
> a single removal.

Ehm yes. That's a good point! I totally didn't figure this out, which makes me think it should've been commented :)

> So the idea here is that *as soon* as we make the switch
> from single device to multi device we must assume we are either active or
> idle. So I think the question is, which one is a better option as the
> default upon switching from single- to multi-device.  I think even if we are
> always registered as an idle obserever, we still have to choose between
> syncInterval = MULTI_DEVICE_ACTIVE_SYNC and MULTI_DEVICE_IDLE_SYNC upon
> switching to multi-device.

Indeed, but if we're always registered as an idle observer, we will at least have the information that will allow us to pick the right one, rather than assuming. Let's say updateClientMode() finds that we go from 1 to more clients, it can just call adjustSyncInterval(), right? That will know which one to pick since it looks at the idle flag.

Anyway, if we'd always be have an idle observer registered, we'd not only be more correct (in what is admittedly an edge case...), but I think the code would also be a lot simpler and less confusing (at least to me :p), and we'd get to reuse stuff like adjustSyncInterval(). Does that make sense?

Comment 27

[Marina Samuel [:emtwo]]

Attachment: Part 4 (v2): sync intervals

If I think of any more tests I will add them.

Comment 28

[Philipp von Weitershausen [:philikon]]

Comment on attachment 541771 [details] [diff] [review]
Part 4 (v2): sync intervals

This looks great and is almost in a landable state! So the following is criticism on a very high level. I have one minor suggestion regarding the numClients stuff:

>--- a/services/sync/modules/service.js
>+++ b/services/sync/modules/service.js
>@@ -79,16 +79,18 @@ Cu.import("resource://services-sync/main
> function WeaveSvc() {
>   this._notify = Utils.notify("weave:service:");
> }
> WeaveSvc.prototype = {
> 
>   _lock: Utils.lock,
>   _locked: false,
>   _loggedIn: false,
>+	
>+  numClients: 0,

I've looked at where this attribute is actually used. It's used mostly in SyncScheduler, only once in Service. So let's move it to SyncScheduler. Then we can get replace `if (this multiDevice)` with `if (this.numClients > 1)`.


The rest of my criticism solely concerns tests:

>+add_test(function test_syncapplied_observer() {

This test doesn't so much test the SyncScheduler as the SyncEngine (specifically _processIncoming). So I think this test function should live in test_syncengine_sync.js.

...

>+  } finally {
>+    SyncScheduler.hasIncomingItems = false;
>+    server.stop(run_next_test);
>+    Svc.Prefs.resetBranch("");
>+    Records.clearCache();
>+  }
>+});

Don't forget to unregister the observer again. Best is you give the function a name so you can refer to it in the `finally` block, e.g.:

 Svc.Obs.remove("weave:engine:sync:applied, onApplied)


>+add_test(function test_sync_observers_call_adjustSyncInterval() {
...
>+  
>+  let adjustSyncIntervalCalls = 0;
>+  SyncScheduler._adjustSyncInterval = SyncScheduler.adjustSyncInterval;
>+  SyncScheduler.adjustSyncInterval = function() {
>+    adjustSyncIntervalCalls++;
>+    SyncScheduler._adjustSyncInterval();
>+  };
>+
...
>+
>+    _("Test successful sync calls adjustSyncInterval");
>+    Service.sync();
>+    do_check_eq(syncSuccesses, 1);
>+    do_check_eq(syncFailures, 0);
>+    // One call for updateClientMode & one for sync:finish
>+    do_check_eq(adjustSyncIntervalCalls, 2);

This doesn't really test all that much, just that SyncScheduler.adjustSyncInterval() was called. What if we refactor adjustSyncInterval, scheduleNextSync, etc.? Then this number might not have much meaning anymore. We should really test the effects of adjustSyncInterval() if we can. We could fake SyncScheduler.idle and numClients for various settings (like in test_adjustSyncInterval() below), then run a sync, and then verify that SyncScheduler.syncInterval was changed appropriately.

We just need to make sure the idle service doesn't interfere, so we probably want to unregister SyncScheduler as an idle observer at the top of the test just to be safe. (This would already be needed for test_adjustSyncInterval() anyway.)

>+    _("Test unsuccessful sync calls adjustSyncInterval");
>+    Service.lockedSync = Service._lockedSync;
>+    Service._lockedSync = function () {
>+      // Force a sync fail.
>+      Service._loggedIn = false;
>+      Service.lockedSync();
>+    };

This is a bit confusing. I suggest assigning the original Service._lockedSync method to a local variable. You just need to make sure you call it with the right 'this' object:

  let origLockedSync = Service._lockedSync;
  Service._lockedSync = function () {
    // Force a sync fail.
    Service._loggedIn = false;
    origLockedSync.call(Service);
  })

>@@ -162,18 +180,16 @@ add_test(function test_scheduleNextSync(
> 
> 
>   _("Test setting sync interval when nextSync != 0");
>   // Schedule next sync for some time in the future
>   expectedInterval = 5000;
>   initial_nextSync = SyncScheduler.nextSync = Date.now() + expectedInterval;
>   SyncScheduler.scheduleNextSync();
> 
>-  //Test nextSync value was changed.
>-  do_check_neq(SyncScheduler.nextSync, initial_nextSync);
>   syncInterval = SyncScheduler.nextSync - Date.now();
>   _("Sync Interval: " + syncInterval);

Why are you removing that check? (Also, where does the magic number for expectedInterval come from?)

>+add_test(function test_client_sync_finish_calls_updateClientMode() {
>+  let updateClientModeCalled = 0;
>+	SyncScheduler._updateClientMode = SyncScheduler.updateClientMode;
>+	SyncScheduler.updateClientMode = function() {
>+	  updateClientModeCalled++;
>+	};
>+ 
>+  let server = sync_httpd_setup();
>+  setUp();
>+
>+  try {
>+    Service.login();
>+
>+    Service._syncEngine(Clients);
>+	  do_check_eq(updateClientModeCalled, 1);
>+	} finally {
>+    SyncScheduler.updateClientMode = SyncScheduler._updateClientMode;
>+    server.stop(run_next_test);
>+  }
>+});

Same critique as with test_adjustSyncInterval above: it would be nice if we tested the effect of syncing the clients engine, rather than just the fact that updateClientMode gets called.

(Also, nit: indention)


>+add_test(function test_sync_at_startup() {
>+  syncTriggered = false;
>+  Svc.Obs.add("weave:service:sync:start", function() {
>+    syncTriggered = true;
>+    do_check_true(syncTriggered);
>+    run_next_test();
>+  });

This has a high chance of ending the test before the sync is actually finished. Why not listen for 'weave:service:sync:finished'? Also, the syncTriggered variable is superfluous. If a sync won't be triggered, won't enter the observer anyway, and won't get to call run_next_test(). The test would just hang. Just simply making it to run_next_test() can be a valid test case sometimes :)

Also, don't forget to remove the observer again. The trick is to give the function a name, so you can refer to it in its own function body:

  Svc.Obs.add("weave:service:sync:finished", function onSyncFinished() {
    Svc.Obs.remove("weave:service:sync:finished", onSyncFinished());
    ...
  });

It would be good if you could double check your usage of observers in tests and see if you unregister them all properly. Often times it doesn't do much harm leaving them around, until you try to extend the test somehow and suddenly things break in a weird way. (Sorry for not having caught this earlier.)

Comment 29

[Marina Samuel [:emtwo]]

Attachment: Part 2 (v4): creating SyncScheduler component & tests

Comment 30

[Marina Samuel [:emtwo]]

Attachment: Part 3 (v3): autoconnect changes

* This patch and the last one both have the most recent observer changes & are based off the most recent repo I pulled yesterday.

Comment 31

[Marina Samuel [:emtwo]]

(In reply to comment #28)
> > 
> >   _("Test setting sync interval when nextSync != 0");
> >   // Schedule next sync for some time in the future
> >   expectedInterval = 5000;
> >   initial_nextSync = SyncScheduler.nextSync = Date.now() + expectedInterval;
> >   SyncScheduler.scheduleNextSync();
> > 
> >-  //Test nextSync value was changed.
> >-  do_check_neq(SyncScheduler.nextSync, initial_nextSync);
> >   syncInterval = SyncScheduler.nextSync - Date.now();
> >   _("Sync Interval: " + syncInterval);
> 
> Why are you removing that check? (Also, where does the magic number for
> expectedInterval come from?)

This test is a bit weird, I must admit. expectedInterval is just an arbitrary number I added to Date.now() so we can have a value for nextSync that is some time in the future (I suppose I should add a more detailed comment).  The reason I removed the check is because it checks that nextSync != initial_nextSync.  This was a sketchy thing to test I realized since it could sometimes be true and sometimes false depending on how long it took to execute scheduleNextSync. Before removing the nextSync pref this test was passing but it began failing after removing the nextSync pref.  I assume setting the pref took some extra time which resulted in a visible time difference making this check pass.

However, the same test done a few lines above is fine because nextSync has an initial value of 0 which will definitely be changed after calling scheduleNextSync

Comment 32

[Marina Samuel [:emtwo]]

Attachment: Part 4 (v3): sync intervals

* moved tests to another patch
* numClients moved to SyncScheduler from Service & replaces multiDevice boolean

Comment 33

[Philipp von Weitershausen [:philikon]]

Pushed Parts 1 thru 4 to s-c:

http://hg.mozilla.org/services/services-central/rev/a1fe709acd8c
http://hg.mozilla.org/services/services-central/rev/35a1943f351c
http://hg.mozilla.org/services/services-central/rev/d6d6211d478c
http://hg.mozilla.org/services/services-central/rev/350b072ac101

Also needed a follow-up fix for test_syncengine_sync.js because *someone* forgot to enable that test before running it :p

http://hg.mozilla.org/services/services-central/rev/6078df7eed78

We will still need those additional tests in Part 5, but this is good enough to hand off to QA now.

Comment 34

[Philipp von Weitershausen [:philikon]]

STRs for QA:

* If a machine has been idle for at least 5 minutes, Sync should sync every 60 minutes (Note: the whole machine must be idle and not in use or be touched in any way!)

* If a machine has been idle for at least 5 minutes and now becomes active, Sync should trigger a sync immediately.

* On a machine that's active (non-idle), Sync should sync:
 - every 5 minutes if there was nothing downloaded in the last sync
 - every 1 minute if the last sync downloaded new items

Comment 35

[Philipp von Weitershausen [:philikon]]

I should point out that comment 34 only applies to accounts that have more than 1 device connected. Otherwise Sync will continue to sync once per day.

Comment 36

[Tracy Walker [:tracy]]

Philipp,

which "activities" are the possible triggers here?

Comment 37

[Philipp von Weitershausen [:philikon]]

(In reply to comment #36)
> which "activities" are the possible triggers here?

*All* user activities (moving the mouse, typing on the keyboard, stomping on the foot pedal, etc.) will cause the machine to be non-idle. Idle really means the machine is just sitting there and nobody's using it.

Comment 38

[Tracy Walker [:tracy]]

non idle mac was not syncing every five minutes.  In fact it synced 65 after previous sync.

non idle windows was syncing around every 15 minutes +/- 3-5 minutes.

This is not working as prescribed.

Comment 39

[Philipp von Weitershausen [:philikon]]

Thanks, Tracy. We'll put the merge on hold while we investigate the non-idle case.

Just to be sure, you tried this out with an account with multiple devices, right?

Also, what about the idle case and the resuming activity case? Have you been able to verify those by any chance?

Comment 40

[Tracy Walker [:tracy]]

(In reply to comment #39)
> Thanks, Tracy. We'll put the merge on hold while we investigate the non-idle
> case.

I'll reply to the hand-off email with status.

> Just to be sure, you tried this out with an account with multiple devices,
> right?
Yes, two clients on the account.  Was watching both.

> Also, what about the idle case and the resuming activity case? Have you been
> able to verify those by any chance?
I hadn't checked the 60 min idle case or the idle at least five minutes, then return to active.  I'll run those this morning.

Comment 41

[Marina Samuel [:emtwo]]

So basically the reason the Mac synced after 1 hr is because it was on
MULTI_DEVICE_IDLE_SYNC and it wasn't a problem with the idle observer.
The problem is that right now the default is SyncScheduler.idle = true
at startup and the only time that changes to false is on "back" so
when you start up your browser you are immediately active except there
is no back notification so it never gets out of MULTI_DEVICE_IDLE_SYNC
mode!

The other silly problem was again my stupid mistake of forgetting to
write 'this'. So the call to adjustSyncInterval() under "idle" and
"back" did not have this.

Comment 42

[Tracy Walker [:tracy]]

Did *not* sync automatically after 60 minutes of idle.

It did fire a sync as soon as I moved the mouse.  So >5 minutes idle, become active worked as expected.

Comment 43

[Marina Samuel [:emtwo]]

Attachment: Part 5 (WIP): tests

Comment 44

[Marina Samuel [:emtwo]]

Attachment: bug fixes for sync intervals

Comment 45

[Philipp von Weitershausen [:philikon]]

Comment on attachment 542626 [details] [diff] [review]
bug fixes for sync intervals

Thanks for the fixes. This is a great example of why we need unit tests :). So no r+ yet until we have tests that exercise the "back" and "idle" observer code paths.

Comment 46

[Philipp von Weitershausen [:philikon]]

Attachment: Part 5 (v1): tests

Rather than reviewing the patch with comments here, I addressed a few minor issues in the tests directly in the patch so that Marina could work on tests for Part 6 in the meantime.

Comment 47

[Philipp von Weitershausen [:philikon]]

Attachment: Part 6 (v2): bug fixes

This is Marina's patch with the tests she wrote. It also contains additional fixes + tests for Service.startOver().

Again, rather than writing review comments here I addressed them directly in the patch.

Comment 48

[Philipp von Weitershausen [:philikon]]

Landed Parts 5 and 6:

http://hg.mozilla.org/services/services-central/rev/07e9119186b5
http://hg.mozilla.org/services/services-central/rev/d882daea99cf

This should hopefully be it!

Comment 49

[Richard Newman [:rnewman]]

Drive-by post-review:

Looks generally good. I found the typo which you added to make sure I was paying attention ;)


44  // Some non-defualt values for SyncScheduler's attributes.

Comment 50

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/a1fe709acd8c
http://hg.mozilla.org/mozilla-central/rev/35a1943f351c
http://hg.mozilla.org/mozilla-central/rev/d6d6211d478c
http://hg.mozilla.org/mozilla-central/rev/350b072ac101
http://hg.mozilla.org/mozilla-central/rev/6078df7eed78
http://hg.mozilla.org/mozilla-central/rev/07e9119186b5
http://hg.mozilla.org/mozilla-central/rev/d882daea99cf