984879 - Experiment manager shutdown

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Benjamin Smedberg]

This bug will hold work on making sure that the experiment manager shuts down properly. When we hit profile-before-change and call uninit() the pending task to save the cache will complete (OS.File guarantees it) but other tasks will abort.

Comment 1

[Benjamin Smedberg]

Attachment: Shut down the Experiments object safely, rev. 1

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8392874 [details] [diff] [review]
Shut down the Experiments object safely, rev. 1

Review of attachment 8392874 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/experiments/Experiments.jsm
@@ +274,5 @@
>  
> +    this._shutdown = true;
> +    if (this._pendingTasks.saveToCache) {
> +      return this._pendingTasks.saveToCache;
> +    }

What about _loadFromCache()? Is it a relevant corner-case we need to wait for too?

@@ +410,5 @@
>        try {
>          let responseText = yield this._httpGetRequest(uri);
>          gLogger.trace("Experiments::updateManifest::updateTask() - responseText=\"" + responseText + "\"");
>  
> +        this._checkForShutdown();

How are we protecting the _saveToCache() call that follows?
Can we have a shutdown happening between the async calls that come before _saveToCache?

@@ +448,5 @@
>      }.bind(this));
>    },
>  
>    onDisabled: function (addon) {
> +    this._checkForShutdown();

The shutdown checks in onDisabled and onUninstalled mean that we can get out of sync with the addon states, so we will have to re-sync on start-up.
This bug or follow-up?

Comment 3

[Benjamin Smedberg]

> What about _loadFromCache()? Is it a relevant corner-case we need to wait
> for too?

No. Reading at shutdown can just abort, since we're not going to make any further state changes.

> 
> @@ +410,5 @@
> >        try {
> >          let responseText = yield this._httpGetRequest(uri);
> >          gLogger.trace("Experiments::updateManifest::updateTask() - responseText=\"" + responseText + "\"");
> >  
> > +        this._checkForShutdown();
> 
> How are we protecting the _saveToCache() call that follows?
> Can we have a shutdown happening between the async calls that come before
> _saveToCache?

I don't understand the question. If we're fetching via HTTP, we just treat that as a failed load. If there's a pending _saveToCache task it will run when this task is rejected.

> The shutdown checks in onDisabled and onUninstalled mean that we can get out
> of sync with the addon states, so we will have to re-sync on start-up.
> This bug or follow-up?

Followup. Syncing addon state on startup and checking conditions before starting the experiment each session will be the next bug.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> > How are we protecting the _saveToCache() call that follows?
> > Can we have a shutdown happening between the async calls that come before
> > _saveToCache?
> 
> I don't understand the question. If we're fetching via HTTP, we just treat
> that as a failed load. If there's a pending _saveToCache task it will run
> when this task is rejected.

If shutdown happens "during" |yield this._evaluateExperiments()| in that task and no |_saveToCache()| task is pending, will |_saveToCache()| just fail due to the OS.File usage?

Comment 5

[Benjamin Smedberg]

It might. But really as soon as we know that the cache is dirty we should enqueue _saveToCache so that we wait for it on shutdown...

Comment 6

[Georg Fritzsche [:gfritzsche]]

Maybe we can just move _saveToCache up to right after |this._updateExperiments(data)|?
It will wait on the other pending tasks, so that should be fine.

Comment 7

[Benjamin Smedberg]

We only call _saveToCache if something has changed, right?

Comment 8

[Georg Fritzsche [:gfritzsche]]

Hm, looking at it again it seems not :(
_updateExperiments() and _evaluateExperiments() would need to propagate that information.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8392874 [details] [diff] [review]
Shut down the Experiments object safely, rev. 1

Review of attachment 8392874 [details] [diff] [review]:
-----------------------------------------------------------------

The functions _disableExperiments and updateManifest will be breaking their contract and throwing an error instead of returning a rejected promise in case the _checkForShutdown function throws.  They are both called from _toggleExperimentsEnabled. At first glance it seems that throwing an error will not cause havoc there. I wonder if it's a valid concern or not to have those functions always returning promises.

Comment 10

[Benjamin Smedberg]

Attachment: Shut down the Experiments object safely, rev. 1.1

Indeed, this returns a rejected promise.

Comment 11

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8f86b4160f

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f8f86b4160f