Ensure that update.status is always written before exiting `--backgroundtask backgroundupdate`
Categories
(Toolkit :: Application Update, task)
Tracking
()
People
(Reporter: nalexander, Unassigned)
References
(Blocks 1 open bug)
Details
While testing I observe that the write to update.status (and other writes, potentially) can be blocked as --backgroundtask backgroundupdate exits. This is simply because the asynchrony in the Update Service doesn't cater to the background tasks use case: there has always been such a race, it's just been rare to trigger it in the wild.
To address this, we might:
- use an existing notification to wait for the update to get further along;
- add additional notifications or callbacks to wait for such processing to be complete;
- figure out how to have background tasks wait for all pending promises/tasks/timeouts to resolve before exiting.
I have no idea if point 3) is feasible. I'll investigate with some JS engine folks. In the interim, I'm going to land with a brief delay so that writes can complete while we test and iterate.
|
||
Comment 1•4 years ago
|
||
Hmm, I wonder if addressing Bug 1674277 might help make progress on point 3.
|
Reporter | |
Comment 2•4 years ago
|
||
I've also thought of another point:
4. Ensure that we wait a tick/JS cycle before exiting, so that any chained promises/microtasks can resolve before we bail out. (We could even wait until idle before we exit, at some risk.) I think that all of the notifications from AppUpdater.jsm are synchronous so this might just be enough.
(Of course, Bug 1674277 might factor into this as well.)
|
|
||
Updated•4 years ago
|
|
|
||
Comment 3•4 years ago
|
||
Since, at the moment, I cannot reproduce this problem or find any code that seems like it would cause this problem, I'm going to unassign this from myself.
|
|
Reporter | |
Comment 4•4 years ago
|
||
NI to me to dig into this, 'cuz I have some thoughts about how I was seeing this.
|
|
Reporter | |
Updated•4 years ago
|
|
|
Reporter | |
Updated•4 years ago
|
|
|
Reporter | |
Comment 5•4 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #4)
NI to me to dig into this, 'cuz I have some thoughts about how I was seeing this.
I finally dug into this, and I can no longer witness this particular issue. I'll close this out; we can always bring it back in the future if we get a bead on this issue.
|
|
||
Comment 6•4 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #5)
I finally dug into this, and I can no longer witness this particular issue. I'll close this out; we can always bring it back in the future if we get a bead on this issue.
Can I remove the sleep at the end of the task then?
|
|
Reporter | |
Comment 7•4 years ago
|
||
(In reply to Kirk Steuber (he/him) [:bytesized] from comment #6)
(In reply to Nick Alexander :nalexander [he/him] from comment #5)
I finally dug into this, and I can no longer witness this particular issue. I'll close this out; we can always bring it back in the future if we get a bead on this issue.
Can I remove the sleep at the end of the task then?
I would not do so until we have movement on https://bugzilla.mozilla.org/show_bug.cgi?id=1703572.
Description
•