Closed Bug 984879 Opened 7 years ago Closed 7 years ago

Experiment manager shutdown

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: benjamin, Assigned: benjamin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8392874 - Flags: review?(felipc)
Attachment #8392874 - Flags: feedback?(georg.fritzsche)
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?
Attachment #8392874 - Flags: feedback?(georg.fritzsche) → feedback+
> 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.
(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?
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...
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.
We only call _saveToCache if something has changed, right?
Hm, looking at it again it seems not :(
_updateExperiments() and _evaluateExperiments() would need to propagate that information.
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.
Indeed, this returns a rejected promise.
Attachment #8392874 - Attachment is obsolete: true
Attachment #8392874 - Flags: review?(felipc)
Attachment #8393011 - Flags: review?(felipc)
Attachment #8393011 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/7f8f86b4160f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.