Abort pending importJSONDump tasks in the worker at shutdown
Categories
(Firefox :: Remote Settings Client, defect, P1)
Tracking
()
People
(Reporter: Gijs, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
The beta numbers in https://bugzilla.mozilla.org/show_bug.cgi?id=1605973 and the last few comments there suggest that the worker is starting off significant indexeddb tasks that we then can't finish in time.
We should rewrite the importJSONDump code so its work happens in one transaction, and then abort that transaction when we realize shutdown is happening.
Assignee | ||
Comment 1•4 years ago
|
||
To use a single transaction for importJSONDump
, this commit:
- changes IDBHelpers'
executeIDB
method to take either a string or array
pointing toobjectStore
s that the caller wants to use. - uses that from RemoteSettingsWorker to start a single transaction using both
therecords
and thetimestamps
store - updates
bulkOperationHelper
to take an optionalcompletion
callback, in
addition to the rejection callback, to be called when all the bulk
operations are complete - uses that optional argument from RemoteSettingsWorker's
importDumpIDB
(the actual implementation of IDB access fromimportJSONDump
) to first
bulk-import the actual records, and then update the timestamp stored for
that remote settings collection.
Then to abort that single transaction, this commit:
- stores pending transactions in a set, similar to what Database.jsm already
does, and removes items from that set when thepromise
fromexecuteIDB
either resolves or rejects. - adds a
prepareShutdown
action on the RemoteSettingsWorker'sAgent
class,
to be called by the jsm side of the worker manager when shutdown happens.
When called, it iterates over the pending transactions and aborts all of
them.
This also sets agShutdown
flag. - ensures that where code
await
s in the middle of an operation, it stops
(throws) immediately ifgShutdown
has been set. - adds a test to test_shutdown_handling.js to verify that this mechanism now
stops pending import tasks in the worker.
Finally, as a driveby, fixes an oversight in test_remote_settings_worker.js
where the second .get()
call wasn't actually testing whether the
importJSONDump
call in the worker had succeeded, because if the collection
was empty it would do the import itself, which I realized when I used similar
code in the shutdown test...
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/52e5c30dc235 abort importJSONDump tasks in the remote settings worker at shutdown, r=leplatrem,asuth
Comment 3•4 years ago
|
||
bugherder |
Assignee | ||
Comment 4•4 years ago
|
||
Comment on attachment 9146592 [details]
Bug 1636256 - abort importJSONDump tasks in the remote settings worker at shutdown, r?leplatrem!,asuth!
Beta/Release Uplift Approval Request
- User impact if declined: Shutdown hangs (ie shutdown crashes)
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: Bug 1635408
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This is a relatively small JS-only change that improves the shutdown handling of the remote settings worker existing shutdown blocker. We're seeing hundreds of these hangs/crashes on release, and I'd like us to nip those in the bud (and, if there remain significant numbers of 77 crashes, to have time to address them in 78 given it's an ESR cycle). I'd like this to land together with the patch in bug 1635408. Since the changes in this bug and bug 1635408, the crashes on nightly have reduced to 0, but it's hard to be sure how strong the effect will be on beta/release given the difference in number of users.
- String changes made/needed: nope
Comment 5•4 years ago
|
||
Comment on attachment 9146592 [details]
Bug 1636256 - abort importJSONDump tasks in the remote settings worker at shutdown, r?leplatrem!,asuth!
Approve for 77 beta 7, thanks.
Comment 6•4 years ago
|
||
bugherder uplift |
Description
•