1450152 - Don't require a restart after flipping the buffered engine pref

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Lina Butler (ex-Mozilla)]

Thom brought this up at our standup. It's annoying to have to restart Firefox when enabling or disabling the buffered engine, and all too easy to accidentally forget (see bug 1440518, comment 1, for example). This would also help with staged rollout and rollback: we could switch folks over immediately, instead of waiting for them to restart.

Thom suggested adding a "delegating engine" that proxies calls to the right engine based on the pref. We'd want to make sure we don't change the engine in the middle of a sync, of course.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime

Review commit: https://reviewboard.mozilla.org/r/233394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233394/

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

I did this more generically than I initially planned on, since I suspect that in the future (when we're moving engines to use mentat or whatever) we also may want to have other cases where what engine is used gets controlled by a preference.

Comment 3

[Lina Butler (ex-Mozilla)]

Comment on attachment 8964695 [details]
Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime

https://reviewboard.mozilla.org/r/233394/#review239014

This looks awesome, thanks, Thom!

::: services/sync/modules/engines.js:512
(Diff revision 1)
>        engines.push(engine);
>      }
>      return engines;
>    },
>  
> +  async switchAlternatives() {

Let's add a comment that this is called just before every sync.

::: services/sync/modules/engines.js:530
(Diff revision 1)
> +      } catch (e) {
> +        this._log.warn(`Failed to unregister previous ${name} engine`, e);
> +      }
> +      info.lastValue = prefValue;
> +      let engineType = prefValue ? info.whenTrue : info.whenFalse;
> +      await this.register(engineType);

Nit: Let's `register` before setting `lastValue`, in case `register` throws? The mirror connects to the database lazily, so it doesn't matter much, but just in case.

::: services/sync/modules/engines.js:649
(Diff revision 1)
>  
>        this._log.error(`Could not initialize engine ${name}`, ex);
>      }
>    },
>  
> -  async unregister(val) {
> +  async unregister(val, removeAlternatives = true) {

Nit: This might be better as an options argument, but I don't have a strong feeling one way or the other.

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8964695 [details]
Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime

https://reviewboard.mozilla.org/r/233394/#review239014

> Nit: Let's `register` before setting `lastValue`, in case `register` throws? The mirror connects to the database lazily, so it doesn't matter much, but just in case.

Hm, this was intentional, so that we don't retry every time if register throws. Although maybe we want to?

Alternatively, maybe the best semantics would probably be to stick with the old engine if register throws... Although then we'd go through this hassle each sync, and would introduce states like "they flipped the pref and synced but are still using the old engine".

I guess it's probably worth making sure that whatever state we get into here, we don't end up doing a bunch of pointless resyncs every time if register throws.

> Nit: This might be better as an options argument, but I don't have a strong feeling one way or the other.

Eh... I think the only caller that is likely to want to use removeAlternatives = false is switchAlternatives. Otherwise we're likely to re-add it next sync, which seems confusing and error prone.

Making this into an options argument implies to me that you may want to use these options, but maybe it's just me. Maybe it would be better if there were a separate `_unregister` method that *just* unregisters the engine and doesn't remove the alt info, and then both `unregister` and `switchAlternatives` would use that (and `unregister` would take no arguments, and always remove both).

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8964695 [details]
Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233394/diff/1-2/

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8964695 [details]
Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime

https://reviewboard.mozilla.org/r/233394/#review239912

LGTM, thanks.

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8964695 [details]
Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233394/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ec8c31cacc4
Handle changing services.sync.engine.bookmarks.buffer at runtime r=kitcambridge,markh

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1ec8c31cacc4