The first step in getting Sync to work without nested event loops is to have the engines themselves and the Sync "service" work with promises instead of nested event loops. This will "break the back" of the effort to remove all explicit nested event-loops.

Bug 1210296 - Get Sync engines and the Sync service working with promises/tasks. r?rnewman. This patch focuses on the Sync service itself, plus all the engines, working using Task.jsm. None of the engines now explicitly spin an event-loop. Places that do spin are: * resource.js - it uses nsIStreamListener, which must not return until all the available data is read. This constraint means we either need to buffer the response in memory or have thousands of promises in-flight at once. For now, we just spin on the promises before returning (but later we will just work out how to use truly async code to read the channel so we only need to read what we can consume) * "authenticators" for resource.js (ie, _getAuthenticationHeader etc) * observer notifications (typically in the trackers) spin when calling promise-returning functions. This is fixed ina subsequent patch. * some tests, including TPS * Some "legacy identity" functions (changePassword, checkAccount, createAccount) Most of these are fixed in subsequent patches. The patch is huge, but most of the changes are "mechanical" - ie, the addition of Task.async for function declarations and the use of "yield" for functions that now return promises.

Mark, can I get this in a Splinter/attached patch rather than ReviewBoard? RB can't handle a patch of this size without spinning itself to death computing label icons. Filed Bug 1213122 to make RB less painful.

(In reply to Richard Newman [:rnewman] from comment #2) > RB can't handle a patch of this size without spinning itself to death > computing label icons. Oh, the irony :)

Comment on attachment 8671718 [details] [diff] [review] 0001-Bug-1210296-Get-Sync-engines-and-the-Sync-service-wo.patch Review of attachment 8671718 [details] [diff] [review]: ----------------------------------------------------------------- Large chunks of this got pretty skimming review. The key bits got more careful attention. Thanks for keeping things so mechanical! Overall comments: * Fucking hell, promise code is hard to read. Eleventeen different ways to make them, combine them, consume them, and so hard to get right. * Does this actually make things better? For example, does this fix Bug 697649? * Re resource.js: is this a good opportunity to switch to rest.js/SyncStorageRequest, which is more modern? We intended rest.js to kill resource.js, but never finished the job. * I'm concerned that this will break third-party Sync engines. If we can shim to avoid that, or find add-ons that need to be fixed (AMO lets us search, right?), then let's do so. And let's document this change somewhere. * Please test the hell out of this. One of the things that's easy to screw up with promise code is error handling! * What's next? Re-chaining things to push yields higher up the stack? Something else? File follow-ups! I'm going to r+ this now, with the understanding that there's more work and testing to do before landing; there's a limit to what code review can achieve. Good job powering through with this! ::: services/common/async.js @@ +142,5 @@ > }, > > + /* > + * Wait for a promise to resolve while spinning an event loop. > + * This is an interim step on the way to removing call nested event-loop "call nested" -> "all nested"? @@ +230,5 @@ > }, > + > + /** > + * A "tight loop" of promises can still lock up the browser for some time. > + * Periodically waiting for a promise returned by this function will solve THE TEARS THEY FALL LIKE RAIN ::: services/common/tests/unit/test_async_querySpinningly.js @@ +4,5 @@ > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://services-common/async.js"); > Cu.import("resource://services-common/utils.js"); > > +// XXX - note that this file used to test 'querySpinningly', but since the Doesn't need the XXX. ::: services/sync/modules/engines.js @@ +343,5 @@ > + yield this.remove(record); > + else { > + let exists = yield this.itemExists(record.id); > + if (!exists) > + yield this.create(record); Big concern: third parties have written their own Sync engines, most notably Adblock Plus. Does a change like this break those engines? @@ +643,5 @@ > eEngineAbortApplyIncoming: "error.engine.abort.applyincoming", > > + // A function to perform deferred initialization of the engine - useful when > + // initialization functions are async/promise based. Note that typically you > + // want this.whenInitialized - calling this funciton will re-initialized. function will re-initialize. @@ +1267,2 @@ > */ > _findDupe: function (item) { This in particular will bust third-party clients, I'd expect. @@ +1627,5 @@ > > removeClientData: function () { > // Implement this method in engines that store client specific data > // on the server. > + return Promise.resolve(); And this. ::: services/sync/modules/engines/bookmarks.js @@ +221,5 @@ > + // XXX - we used to override _sync() and call PlacesUtils.bookmarks.runInBatchMode > + // before calling the base implementation - but runInBatchMode kinda sucks > + // for our purposes here as it doesn't support async operations. So for > + // now, we take the perf hit of not running in batch mode. > + // We really just need runInBatchMode() to support promises - bug 890203. Leave cross-referencing comments in that bug! ::: services/sync/modules/policies.js @@ +699,5 @@ > syncAndReportErrors: function syncAndReportErrors() { > this._log.debug("Beginning user-triggered sync."); > > this.dontIgnoreErrors = true; > + return this.service.sync(); Woohoo ::: services/sync/modules/resource.js @@ +378,2 @@ > * > + * 'Resource' is recommended for new code. Wow there are a lot of code snippets we're making stale :) @@ +558,5 @@ > + // for the promise to resolve. Ideally we need to work out the best way > + // to do things truly async (ie, to read the stream as fast as we can > + // consume it, as opposed to this "listener" model that insists we > + // consume it as fast as it can be produced.) > + Async.promiseSpinningly(this._onProgress()); You should see the terrifying code we have in android-sync to do exactly that. Make sure you have bugs on file for these things… ::: services/sync/modules/util.js @@ +98,5 @@ > + return null; > + } > + try { > + return Task.spawn(func.call(thisArg)).then( > + null, ex => exceptionHandler(ex) My brain hurts. ::: services/sync/tests/unit/head_helpers.js @@ +207,5 @@ > } > } > + > +// A helper that waits for an observer notification. Returns a promise that > +// is resolved with the subject and data of the notification. There's code a lot like this in Weave.js. Can we unify?

I'm not planning on doing further work on this until the promises/task tooling improves such that we get sane stack traces by default - with these patches we typically get stack traces that list only task or promise internals and make no reference to the actual code in error.

Should we consider going back to this since async/await is landing in Firefox 52?

Do you know the state of async stacks in Nightly? Two months ago, stacks in xpcshell tests only pointed to the calling function, which was less informative than tasks. But that was in test code, and `add_task` still uses Task.jsm under the hood, so I wonder if it's different or has gotten better.

Edouard is picking this up again, and it sounds like we have async stack traces now! (Promises can still be a problem depending on how they're chained, but that's OK, and we can ask the JS team for help).

This patch might look scary to review (I mean, 101 files...) but the changes fairly mechanical. I kept Async.promiseSpinningly in problematic areas that require more thought: - Getter/Setters - Observers/HTTP Channels - Constructors This patch is a base, then we can open following-up bugs and iterate on killing the problematic Async.* calls one by one. I also made sure we are yielding in tight promise-loops so we avoid starving the event loop. Not sure who should review this.

Also this patch breaks TPS hard, it needs another commit before landing.

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review141776 I started on the first pass, and got to engines/addons.js. This is awesome, thanks for taking it on! It'd be great to have Thom and Mark review, too. ::: services/sync/modules-testing/rotaryengine.js:113 (Diff revision 1) > _storeObj: RotaryStore, > _trackerObj: RotaryTracker, > _recordObj: RotaryRecord, > > - _findDupe: function _findDupe(item) { > + async _findDupe(item) { > // This is a semaphore used for testing proper reconciling on dupe First time I've seen the word "semaphore" used this way. TIL! ::: services/sync/modules/addonsreconciler.js:174 (Diff revision 1) > * Accessor for add-ons in this object. > * > - * Returns an object mapping add-on IDs to objects containing metadata. > + * Returns a promise that resolves with an object mapping add-on IDs to > + * objects containing metadata. > */ > get addons() { I have a minor preference for this being a function instead of a getter that initializes the reconciler as a side effect, but I don't feel strongly about it. ::: services/sync/modules/addonsreconciler.js:332 (Diff revision 1) > this._log.info("Refreshing global state from AddonManager."); > - this._ensureStateLoaded(); > + await this._ensureStateLoaded(); > > let installs; > + let addons = await new Promise(res => { > + AddonManager.getAllAddons(addons => res(addons)); It looks like `AddonManager` supports promises now! \o/ (Bug 987512). ::: services/sync/modules/browserid_identity.js:280 (Diff revision 1) > // is resolved at the start of authentication, but we don't want to fire > // this event or start the next sync until after authentication is done > // (which is signaled by `this.whenReadyToAuthenticate.promise` resolving). > this.whenReadyToAuthenticate.promise.then(() => { > Services.obs.notifyObservers(null, "weave:service:setup-complete"); > return new Promise(resolve => { Weave.Utils.nextTick(resolve, null); }) Can you use your `promiseYield` helper here, too? ::: services/sync/modules/collection_validator.js:136 (Diff revision 1) > // Returns an object containing > // problemData: an instance of the class returned by emptyProblemData(), > // clientRecords: Normalized client records > // records: Normalized server records, > // deletedRecords: Array of ids that were marked as deleted by the server. > compareClientWithServer(clientItems, serverItems) { Let's file a follow-up bug to make this async. ::: services/sync/modules/engines.js:374 (Diff revision 1) > * > * @param record > * Record to apply > */ > - applyIncoming(record) { > + async applyIncoming(record) { > if (record.deleted) Nit: While you're here, mind adding braces around the `if` bodies? ::: services/sync/modules/engines.js:394 (Diff revision 1) > * > * @param record > * The store record to create an item from > */ > - create(record) { > + async create(record) { > throw "override create in a subclass"; For another follow-up bug: let's make sure we throw `Error` objects, not strings. ::: services/sync/modules/engines.js:873 (Diff revision 1) > - let cb = (error) => { > - if (error) { > - this._log.error("Failed to read JSON records to fetch", error); > - } > - } > // Coerce the array to a string for more efficient comparison. *blink* Fascinating! ::: services/sync/modules/engines.js:948 (Diff revision 1) > /* > * Returns a changeset for this sync. Engine implementations can override this > * method to bypass the tracker for certain or all changed items. > */ > - getChangedIDs() { > + async getChangedIDs() { > return this._tracker.changedIDs; Sweet, I think this means we can remove the spinning in `Tracker::changedIDs`. Definitely material for a follow-up bug, though. :-) ::: services/sync/modules/engines.js:1232 (Diff revision 1) > } > > if (applyBatch.length == self.applyIncomingBatchSize) { > - doApplyBatch.call(self); > + await doApplyBatch.call(self); > } > self._store._sleep(0); Can this be replaced with `promiseYield`? (Or removed, since you `promiseYield` in the loop below, too).

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review141776 > Nit: While you're here, mind adding braces around the `if` bodies? Let's do that in the whole services/ dir in a follow-up bug: |(if \(.+\)|else)\n| for reference Eslint could also do it automatically with --fix: http://eslint.org/docs/rules/curly

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review141776 > Let's do that in the whole services/ dir in a follow-up bug: > |(if \(.+\)|else)\n| for reference > Eslint could also do it automatically with --fix: http://eslint.org/docs/rules/curly Sweet!

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review141860 I just had a quick look and I think it looks great - but there's a real risk here I'm not sure how to address: * With the formautofill work, we need to maintain some degree of backwards compatibility for them - imagine in Firefox 65, the current version of their addon might be expected to work back to Firefox 61. This means that unlike our current workflow, we can't just make breaking changes and updating all our engines - we suddenly also need to consider how their engine will work with older releases. * There are current 3rd party engines that work with Sync (eg, ghostery, stylish, abp, etc), and their engines are likely to be broken by these changes. These old-style XUL addons will be broken in 57, so that would probably be a safe time to ignore those 3rd-party engines. BUT, formautofill is aiming for 56. So if we make these breaking changes in 57, I expect we'll find we've broken formautofill sync if they make an addon update in the 57 cycle but want to push it to 56 users. It *might* be the case that we could make this change in 56, breaking the old XUL addon sync engines in that release - the addons will still work in general, but only for 1 more cycle - the authors must see the writing on the wall. However, I'd be reluctant to do this without at least attempting to reach out to the addons we know have sync engines. It's tricky :( ::: browser/base/content/sync/aboutSyncTabs.js:1 (Diff revision 1) > /* This Source Code Form is subject to the terms of the Mozilla Public I think we should get a bug on file to remove this - we only kept it as it was the only way to access "cloud sync", but that's now removed (and we've been closing bugs for about:sync-tabs as WONTFIX. ::: services/sync/modules/addonsreconciler.js:229 (Diff revision 1) > * will be used. > - * @param callback > - * Function to be invoked on save completion. No parameters will be > - * passed to callback. > */ > - saveState: function saveState(path, callback) { > + async saveState(path = null) { couldn't we just make DEFAULT_STATE_FILE the default arg? ::: services/sync/modules/addonutils.js:161 (Diff revision 1) > * Addon instance to uninstall. > - * @param cb > - * Function to be invoked when uninstall has finished. It receives a > - * truthy value signifying error and the add-on which was uninstalled. > */ > - uninstallAddon: function uninstallAddon(addon, cb) { > + async uninstallAddon(addon) { this function doesn't need to be declared async, right? (It doesn't really matter that it is, but I can imagine a future eslint rule that enforces, say, that an async function must await) ::: services/sync/modules/collection_validator.js:136 (Diff revision 1) > // Returns an object containing > // problemData: an instance of the class returned by emptyProblemData(), > // clientRecords: Normalized client records > // records: Normalized server records, > // deletedRecords: Array of ids that were marked as deleted by the server. > compareClientWithServer(clientItems, serverItems) { or just do it in this bug - this seems relatively easy to asyncify? (but yeah, happy for it to be in another bug if I'm missing some subtlety) ::: services/sync/modules/engines.js:312 (Diff revision 1) > return Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > }); > } > Store.prototype = { > > - _sleep: function _sleep(delay) { > + _sleep(delay) { we should try and kill this too - I think *everyone* calls it with zero, so replacing all calls with promiseYield probably makes sense ::: services/sync/modules/engines.js:717 (Diff revision 1) > let tracker = new this._trackerObj(this.Name, this); > this.__defineGetter__("_tracker", () => tracker); > return tracker; > }, > > - sync() { > + async sync() { ditto here and below - I'm not sure an async function that doesn't wait makes sense. I also got advice on #developers that not making it async might be slightly faster as it saves an implicit .resolve ::: services/sync/modules/engines.js:770 (Diff revision 1) > }; > > this.SyncEngine = function SyncEngine(name, service) { > Engine.call(this, name || "SyncEngine", service); > > - this.loadToFetch(); > + Async.promiseSpinningly(this.loadToFetch()); it might make sense to introduce a new initialize function on engines that returns a promise. ::: services/sync/modules/engines.js:1639 (Diff revision 1) > > for (let id of successful) { > this._modified.delete(id); > } > > - this._onRecordsWritten(successful, failed); > + Async.promiseSpinningly(this._onRecordsWritten(successful, failed)); It should be fairly easy to get rid of this too by making postQueue.enqueue and postQueue.flush async?

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review141776 > First time I've seen the word "semaphore" used this way. TIL! slight off-topic for this patch, but ISTM "sentinal" would be the correct term? (well, maybe the incorrect spelling of the correct term? ;) "sentinel"

Patch amended with your comments, thank you both for your feedback. I think this is ready for a formal review. > It *might* be the case that we could make this change in 56, breaking the old XUL addon sync engines in that release - the addons will still work in general, but only for 1 more cycle - the authors must see the writing on the wall. However, I'd be reluctant to do this without at least attempting to reach out to the addons we know have sync engines. This is the less painful solution imo, and hopefully by then the big addons will have Webextension versions.

About 5 add ons we need to worry about. Install these add ons on your test profile and demonstrate that they aren't broken.

I tested the 3 addons you cited Mark: - AdBlock Plus: Sync limited to filter lists, works with this patch. - OK - Stylish: No Sync support, checked the source code, nothing - OK - Ghostery: Uses their own sync solution - OK I tried to run a Spark analysis to find more 3rd party engines but failed miserably. I feel more confident about this patch though.

More addons tests: - Greasemonkey: No problem syncing userscripts - OK - Stylishsync: Could not make it work (w and w/o patch), from the source code the engine looks OK. That's it for the big 3rd party sync engines.

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review143772 This is heading in the direction we want to end up at, so f+, but it makes me very nervous :) We need a story for Async.isShutdownException too, which I guess might need to be in promiseYield()? More generally, this is a massive and risky patch that is very difficult to review - I only got as far as the tests and couldn't face page 2 of the changes. I'm still wondering if there is scope to split it - for example, one problematic part of this is the spinning in resource.js, and we are eventually going to need a story for that. ISTM that it should be possible to split that, even before the consumers of the data are promise-based. There are probably other things that could be split too. Working out how to split it will give it the best chance of landing - it is going to take literally hours for a thorough review of this (it took me well over an hour to quickly scan the non-test code!) Also, you need to perform *very thorough* testing, both for correctness and performance. That's almost certainly going to require some test scripts to populate a profile with syncable data (Kit can point you at hs bookmarks one, but something similar will be needed for history, autofill, passwords, etc) and fully stress test it (eg, first syncs, incremental syncs, syncs after reset, etc). We can't rely on QA/Softvision for this depth of testing (but *will* want to get them involved for testing regardless, so should make sure they are able to put time aside as it gets close to landing) ::: services/sync/modules/SyncedTabs.jsm:179 (Diff revision 2) > - log.info("Doing a tab sync."); > + log.info("Doing a tab sync."); > - Weave.Service.sync(["tabs"]); > - resolve(true); > + await Weave.Service.sync(["tabs"]); > + return true; > - } catch (ex) { > + } catch (ex) { > - log.error("Sync failed", ex); > + log.error("Sync failed", ex); > - reject(ex); > + return false; this is a semantic change that seems unnecessary as part of this patch. ::: services/sync/modules/addonsreconciler.js:511 (Diff revision 2) > }, > > /** > * Obtains the set of all known Sync GUIDs for add-ons. > * > - * @return Object with guids as keys and values of true. > + * @return Promise that resolves to an object with guids as keys and values of true. this raises a stylistic issue - you've documented this async function as returning a promise, but it's not clear that's been done everywhere (or maybe it has?) I don't have a strong opinion other than "we should be consistent" - but my first reaction is that a function explicitly declared as async doesn't need to document it returns a promise. ::: services/sync/modules/addonsreconciler.js:551 (Diff revision 2) > * > * This is called internally by anything that accesses the internal data > * structures. It effectively just-in-time loads serialized state. > */ > - _ensureStateLoaded: function _ensureStateLoaded() { > + async _ensureStateLoaded() { > if (this._stateLoaded) { I wonder if this would be safer with an explicit promise - something like: ``` _ensureStateLoaded() { if (!this._promiseStateLoaded) { this._promiseStateLoaded = this.loadState(); } return this._promiseStateLoaded; } ``` ? OTOH though, if each engine has some async initialize method, the addons engine could wait for the loaded promise at that time. That would probably make these changes simpler, especially in the listener, as you could just use this.addons directly without converting that to getAddons() (and thus avoid one of the spins in the listener) ::: services/sync/modules/collection_validator.js:137 (Diff revision 2) > // problemData: an instance of the class returned by emptyProblemData(), > // clientRecords: Normalized client records > // records: Normalized server records, > // deletedRecords: Array of ids that were marked as deleted by the server. > compareClientWithServer(clientItems, serverItems) { > - clientItems = clientItems.map(item => this.normalizeClientItem(item)); > + clientItems = clientItems.map(item => Async.promiseSpinningly(this.normalizeClientItem(item))); In my previous comment I suggested we should be able to hoist this spinning up ::: services/sync/modules/engines.js:331 (Diff revision 2) > */ > - applyIncomingBatch(records) { > + async applyIncomingBatch(records) { > let failed = []; > + let yieldCounter = 0; > for (let record of records) { > + if (++yieldCounter % 50 === 0) { // Yield from time to time to keep the UI responsive We might need to move this into the try block if we end up using promiseYield to handle the shutdown semantics correctly. ::: services/sync/modules/engines.js:668 (Diff revision 2) > } > Engine.prototype = { > // _storeObj, and _trackerObj should to be overridden in subclasses > _storeObj: Store, > _trackerObj: Tracker, > + _initialized: false, I'm not sure we really need this check TBH and it could just be a noop (although I don't feel strongly about it) ::: services/sync/modules/engines.js:1300 (Diff revision 2) > // in a single request, even for desktop, to avoid hitting URI limits. > batchSize = isMobile ? this.mobileGUIDFetchBatchSize : > this.guidFetchBatchSize; > > while (fetchBatch.length && !aborting) { > + // We yield every batch to keep the UI responsive batches are big, so I suspect we want to yield more often? ::: services/sync/modules/engines.js:1948 (Diff revision 2) > * shouldn't upload as part of a weak reupload (items that have changed, > * for example). > */ > - buildWeakReuploadMap(idSet) { > + async buildWeakReuploadMap(idSet) { > let result = new Map(); > + let yieldCounter = 0; we're reusing this pattern a few times - I wonder if we could abstract it somehow - say an object with a .maybeYield() function - the initial naive implementation could stick with a "yield every 50 calls" implementation, but it might make it easier to refactor later should a smarter implementation become apparent. eg: let yielder = new JankYielder(); while (true) { yielder.maybeYield(); } or similar. ::: services/sync/modules/engines/addons.js:369 (Diff revision 2) > // the reconciler only, we need to take care of some corner cases. > // > // First, the reconciler could know about an add-on that was uninstalled > // and no longer present in the add-ons manager. > if (!addon) { > - this.create(record); > + await this.create(record); should this just return this.create(record)? ::: services/sync/modules/engines/addons.js:385 (Diff revision 2) > > // We continue with processing because there could be state or ID change. > } > > let cb = Async.makeSpinningCallback(); > this.updateUserDisabled(addon, !record.enabled, cb); converting this to a promise seems fairly reasonable (even if just done locally by passing a new promise's resolve as the callback) ::: services/sync/modules/engines/addons.js:498 (Diff revision 2) > guid); > continue; > } > > this._log.info("Uninstalling add-on as part of wipe: " + addon.id); > - Utils.catch.call(this, () => addon.uninstall())(); > + await Utils.catch.call(this, () => addon.uninstall())(); there appear to be only 2 callers of catch.call in the tree - I'm wondering if we should just kill it? ::: services/sync/modules/engines/bookmarks.js:311 (Diff revision 2) > if (parent) { > yield [tree, parent]; > } > if (tree.children) { > for (let child of tree.children) { > - store._sleep(0); // avoid jank while looping. > + Async.promiseSpinningly(Async.promiseYield()); // avoid jank while looping. this seems unfortunate - it seems worth seeing if we can make a relatively simple change to avoid it. ::: services/sync/modules/engines/bookmarks.js:459 (Diff revision 2) > // Create a mapping of folder titles and separator positions to GUID. > // We do this lazily so that we don't do any work unless we reconcile > // incoming items. > let guidMap; > try { > - guidMap = this._buildGUIDMap(); > + guidMap = Async.promiseSpinningly(this._buildGUIDMap()); ditto here - but I guess a followup is OK ::: services/sync/modules/engines/clients.js:238 (Diff revision 2) > /** > * Low level function, do not use directly (use _addClientCommand instead). > */ > _saveCommands(commands) { > - let cb = Async.makeSpinningCallback(); > - Utils.jsonSave("commands", this, commands, error => { > + try { > + Utils.jsonSave("commands", this, commands); don't we need a promiseSpinningly here? ::: services/sync/modules/engines/clients.js:503 (Diff revision 2) > this._knownStaleFxADeviceIds = null; > delete this.localCommands; > - this._store.wipe(); > - const logRemoveError = err => this._log.warn("Could not delete json file", err); > - Async.promiseSpinningly( > - Utils.jsonRemove("commands", this).catch(logRemoveError) > + await this._store.wipe(); > + try { > + await Utils.jsonRemove("commands", this); > + await Utils.jsonRemove("commands-syncing", this) We should still try the second removal even if the first fails (although I guess that's bad either way - but the existing semantics do that, so here should too) ::: services/sync/modules/engines/forms.js:58 (Diff revision 2) > - return Async.promiseSpinningly(this._promiseSearch(terms, searchData)); > - }, > - > - _updateSpinningly(changes) { > if (!FormHistory.enabled) { > - return; // update isn't going to do anything. > + return Promise.resolve(); // update isn't going to do anything. can't you just return here? ::: services/sync/modules/engines/history.js:298 (Diff revision 2) > records.length = k; // truncate array > > - let handleAsyncOperationsComplete = Async.makeSyncCallback(); > - > if (records.length) { > - blockers.push(new Promise(resolve => { > + await new Promise(resolve => { Now we are waiting for each item to resolve before moving on to the next. I doubt this would change the actual semantics, but it might change the performance. I guess it doesn't matter if we can demonstrate this isn't noticibly slower with many form history items. ::: services/sync/modules/engines/history.js:397 (Diff revision 2) > return true; > }, > > - remove: function HistStore_remove(record) { > + async remove(record) { > this._log.trace("Removing page: " + record.id); > return PlacesUtils.history.remove(record.id).then( I think you may as well await here ::: services/sync/modules/resource.js:403 (Diff revision 2) > try { > channel.QueryInterface(Ci.nsIHttpChannel); > } catch (ex) { > this._log.error("Unexpected error: channel is not a nsIHttpChannel!"); > > - this._onComplete(ex, this._data, channel); > + Async.promiseSpinningly(this._onComplete(ex, this._data, channel)); introduction of promiseSpinningly's here seems wrong and a step backwards - we might need to see if we can think of some alternative (but I'm just hand-waving at the moment) ::: services/sync/modules/service.js:297 (Diff revision 2) > this._clusterManager = this.identity.createClusterManager(this); > this.recordManager = new RecordManager(this); > > this.enabled = true; > > - this._registerEngines(); > + Async.promiseSpinningly(this._registerEngines()); could we have a new startup/initialize promise, and have sync wait for that? (ie, we could still kick it off in onStartup(), but have sync() await for it). It would be a shame if even after this patch, the very first thing sync does as it initializes is to spin an event loop! ::: services/sync/modules/service.js:413 (Diff revision 2) > case "sync:collection_changed": > // We check if we're running TPS here to avoid TPS failing because it > // couldn't get to get the sync lock, due to us currently syncing the > // clients engine. > if (data.includes("clients") && !Svc.Prefs.get("testing.tps", false)) { > - this.sync([]); // [] = clients collection only > + Async.promiseSpinningly(this.sync([])); // [] = clients collection only hrm - this code seems wrong before you touched it. Given the lock, I think it's probably fine to just .sync().catch here? ::: services/sync/modules/service.js:803 (Diff revision 2) > - () => cb(null), > - err => cb(err || "ensureLoggedIn failed") > - ); > - > // Just let any errors bubble up - they've more context than we do! > - cb.wait(); > + this.identity.ensureLoggedIn(); don't we want to await here? This function can still return a rejected promise. ::: services/sync/modules/stages/enginesync.js:198 (Diff revision 2) > let dateStr = Utils.formatTimestamp(new Date()); > this._log.info("Sync completed at " + dateStr > + " after " + syncTime + " secs."); > } > > - this.onComplete(null); > + return null; it looks like this function should just plain |return;| throughout.

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review142808 More scattered comments; working my way through the patch. ::: services/sync/modules-testing/fakeservices.js:42 (Diff revision 2) > let json = typeof obj == "function" ? obj.call(that) : obj; > self.fakeContents["weave/" + filePath + ".json"] = JSON.stringify(json); > - if (callback) { > - callback.call(that); > - } > return Promise.resolve(); Nit: No need to return `Promise.resolve(...)` from an async function. ::: services/sync/modules-testing/rotaryengine.js:42 (Diff revision 2) > this.items = {}; > } > RotaryStore.prototype = { > __proto__: Store.prototype, > > - create: function create(record) { > + async create(record) { Style question: it's OK to `await` a non-`async` function, but do we want to follow that style? On the one hand, it might help keep the diff smaller; on the other, we lose clarity, and these small changes aren't the complicated parts of the patch. WDYT? ::: services/sync/modules/engines.js:601 (Diff revision 2) > * > * @param engineObject > * Engine object used to get an instance of the engine > * @return The engine object if anything failed > */ > - register(engineObject) { > + async register(engineObject) { While you're here, could you make `unregister` async, too, or would you prefer to do that in another bug? ::: services/sync/modules/engines.js:668 (Diff revision 2) > } > Engine.prototype = { > // _storeObj, and _trackerObj should to be overridden in subclasses > _storeObj: Store, > _trackerObj: Tracker, > + _initialized: false, Are there cases we expect an engine to be initialized twice? If not, let's consider dropping the check; duplicate `initialize` calls could indicate a bug. ::: services/sync/modules/engines.js:722 (Diff revision 2) > let tracker = new this._trackerObj(this.Name, this); > this.__defineGetter__("_tracker", () => tracker); > return tracker; > }, > > - sync() { > + async sync() { :-) ::: services/sync/modules/engines.js:836 (Diff revision 2) > applyIncomingBatchSize: DEFAULT_STORE_BATCH_SIZE, > > + async initialize() { > + await Engine.prototype.initialize.call(this); > + await this.loadToFetch(); > + await this.loadPreviousFailed(); Are we guaranteed that initialization will finish before we access `previousFailed` or `toFetch`? I think the answer is "yes", but I wanted to make sure. ::: services/sync/modules/engines.js:1144 (Diff revision 2) > > // Not binding this method to 'this' for performance reasons. It gets > // called for every incoming record. > let self = this; > > - newitems.recordHandler = function(item) { > + newitems.recordHandler = async function(item) { This has so many layers (not your fault!)...`recordHandler` sets `_onProgress`, which is also an async function that we spin in the synchronous `onDataAvailable`. Would it be worth keeping these handlers synchronous, and handle the request side of things in a follow-up? ::: services/sync/modules/engines/bookmarks.js:310 (Diff revision 2) > // Skip root node > if (parent) { > yield [tree, parent]; > } > if (tree.children) { > for (let child of tree.children) { https://github.com/tc39/proposal-async-iteration could help here eventually. (I asked in #jsapi, and was told they're disabled in chrome code for now, while they work out changes to the semantics). ::: services/sync/modules/resource.js:459 (Diff revision 2) > > try { > let httpChannel = req.QueryInterface(Ci.nsIHttpChannel); > - this._onProgress(httpChannel); > + // We don't want to allow too many in-flight promises and don't want to > + // buffer the entire response in memory, so for now we block here waiting > + // for the promise to resolve. Ideally we need to work out the best way I wonder if http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/netwerk/base/nsIRequest.idl#82,89 might help us here. ::: services/sync/modules/resource.js:462 (Diff revision 2) > - this._onProgress(httpChannel); > + // We don't want to allow too many in-flight promises and don't want to > + // buffer the entire response in memory, so for now we block here waiting > + // for the promise to resolve. Ideally we need to work out the best way > + // to do things truly async (ie, to read the stream as fast as we can > + // consume it, as opposed to this "listener" model that insists we > + // consume it as fast as it can be produced.) I can't wait for `fetch` and native streams. It's really unfortunate we have to implement our own backpressure atop these channel listeners.

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review145640 The size and scope of this patch worries me a great deal... It subtly changes the semantics of most of the APIs in sync, and I don't think the failure case would be exercised by tests necessarially -- especially since it might be nondeterministic (or rather, rely on scheduler behavior). In particular, the potential for code to suddenly need to be re-entrant worries me a great deal... That's to say, I don't think we can entirely rely on tests passing to indicate that this doesn't introduce problems (even TPS). Basically, I think we need to land this so that it sits in nightly for a whole / nearly a whole cycle, crashlanding it seems very likely to bite us terribly. FYI: Only skimmed the test changes, but I'm probably ok with them so long as everything still passes reliably (including TPS!!). ::: services/sync/modules/engines.js (Diff revision 2) > } > > if (applyBatch.length == self.applyIncomingBatchSize) { > - doApplyBatch.call(self); > + await doApplyBatch.call(self); > } > - self._store._sleep(0); Seems like we want some yielding to the event loop somewhere to avoid jank, am I missing it? ::: services/sync/modules/engines.js:1948 (Diff revision 2) > * shouldn't upload as part of a weak reupload (items that have changed, > * for example). > */ > - buildWeakReuploadMap(idSet) { > + async buildWeakReuploadMap(idSet) { > let result = new Map(); > + let yieldCounter = 0; 2nding the existance of something along the lines of JankYielder. ::: services/sync/modules/engines/bookmarks.js:311 (Diff revision 2) > if (parent) { > yield [tree, parent]; > } > if (tree.children) { > for (let child of tree.children) { > - store._sleep(0); // avoid jank while looping. > + Async.promiseSpinningly(Async.promiseYield()); // avoid jank while looping. This should be solvable by creating the array we iterate over up front and yielding inside the code doing the iteration. If there's still a performance issue, removing the recursive generator code should fix it (to be clear, iterating over the bookmark tree, adding them to an array should absolutely be fast enough) To be specific, I think you'd want to change the loop to `for (let [node, parent] of Array.from(walkBookmarksRoots(tree)))`, and move the `await Async.promiseYield()` to the body of the loop. ::: services/sync/modules/engines/clients.js:229 (Diff revision 2) > return this._store._remoteClients[id].type == DEVICE_TYPE_MOBILE; > return false; > }, > > _readCommands() { > - let cb = Async.makeSpinningCallback(); > + let commands = Async.promiseSpinningly(Utils.jsonLoad("commands", this)); Is there a reason we seem to be keeping so many of the promiseSpinningly's in clients.js? ::: services/sync/modules/engines/forms.js:86 (Diff revision 2) > - let results = this._searchSpinningly(this._guidCols, query); > + let results = await this._search(this._guidCols, query); > return results.length ? results[0].guid : null; > }, > > - hasGUID(guid) { > + async hasGUID(guid) { > // We could probably use a count function here, but searchSpinningly exists... s/searchSpinningly/search/ ::: services/sync/modules/engines/history.js:236 (Diff revision 2) > }, > > // See bug 468732 for why we use SQL here > _findURLByGUID: function HistStore__findURLByGUID(guid) { > this._urlStm.params.guid = guid; > return Async.querySpinningly(this._urlStm, this._urlCols)[0]; History is the largest collection, so it seems plausible to me that it's the one we spend the most time syncing, so it seems worth checking to see how much work it would be to move to the async history API too... I looked at this recently for TPS and it seemed not to be too much work but I could have been mistaken. ::: services/sync/modules/engines/history.js:298 (Diff revision 2) > records.length = k; // truncate array > > - let handleAsyncOperationsComplete = Async.makeSyncCallback(); > - > if (records.length) { > - blockers.push(new Promise(resolve => { > + await new Promise(resolve => { Agreed, and I think this is likely to be an issue since history (and this is history, not form history) is easily the collection with the most records... ::: services/sync/modules/record.js:786 (Diff revision 2) > // Switch to newline separated records for incremental parsing > coll.setHeader("Accept", "application/newlines"); > > this._onRecord = onRecord; > > - this._onProgress = function(httpChannel) { > + this._onProgress = async function(httpChannel) { I'm concerned about making this function asynchronous since it is not reentrant. It looks like the caller avoids the issue here by calling with a promiseSpinningly, which feels opaque to me (at the very least, a comment explaining this is necessary. ::: services/sync/modules/record.js:999 (Diff revision 2) > this.bytesAlreadyBatched += queued.length; > this.numAlreadyBatched += this.numQueued; > } > this.queued = ""; > this.numQueued = 0; > let response = Async.promiseSpinningly( Why do we still spin here? (Not suggesting there's no reason to, but maybe comment why if there is) ::: services/sync/modules/resource.js:457 (Diff revision 2) > throw ex; > } > > try { > let httpChannel = req.QueryInterface(Ci.nsIHttpChannel); > - this._onProgress(httpChannel); > + // We don't want to allow too many in-flight promises and don't want to I'm not 100% convinced buffering the response in memory is that big of a mistake now that we buffer the deserialized data in memory anyway (or, we often do anyway). I could be missing something though. ::: services/sync/modules/util.js:88 (Diff revision 2) > * MyObj.foo = function() { this._catch(func)(); } > * > * Optionally pass a function which will be called if an > * exception occurs. > */ > - catch: function Utils_catch(func, exceptionCallback) { > + catch(func, exceptionCallback) { This makes a synchronous function into an asynchronous one. Have we made sure all callers catch() are aware that it's result needs to be `await`ed now? Same for the other similar functions like lock and notify. ::: services/sync/tests/unit/head_helpers.js:466 (Diff revision 2) > - if (caughtError) { > + if (caughtError) { > - Svc.Obs.notify("weave:service:sync:error", caughtError); > + Svc.Obs.notify("weave:service:sync:error", caughtError); > - } else { > + } else { > - Svc.Obs.notify("weave:service:sync:finish"); > + Svc.Obs.notify("weave:service:sync:finish"); > - } > + } > - }); > + return submitPromise; Maybe `return await` here to ensure this function ends up on the stack trace? (Unless you're confident it will anyway)

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review143772 > We might need to move this into the try block if we end up using promiseYield to handle the shutdown semantics correctly. Do we? It seems like we are only re-throwing the exception here. > can't you just return here? 60:5 error Async method '_update' expected no return value. consistent-return (eslint) :/

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review145640 > History is the largest collection, so it seems plausible to me that it's the one we spend the most time syncing, so it seems worth checking to see how much work it would be to move to the async history API too... > > I looked at this recently for TPS and it seemed not to be too much work but I could have been mistaken. Let's do that in a follow-up to keep this patch mechanical.

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review148694 Looking good! I think Thom should have a look too ::: services/sync/modules/record.js:737 (Diff revision 3) > // Actually perform the request > resp = await this.get(); > if (!resp.success) { > break; > } > + let length = 0; most of this is just for the .trace, right? I'm not sure we actually need that logging TBH, and it makes this code look more complicated than it actually is. ::: services/sync/modules/resource.js:306 (Diff revision 3) > this._log.debug("Caught exception visiting headers in _onComplete", ex); > } > > - let ret = new String(data); > - ret.url = channel.URI.spec; > - ret.status = status; > + let ret = new String(data); > + ret.url = channel.URI.spec; > + ret.contentLength = headers["content-length"] || "unknown"; which means you could probably avoid this change too, right? (and regardless, given you return headers, I'd say it's fine for callers to headers["content-length"] themselves) ::: services/sync/tests/unit/head_http_server.js:321 (Diff revision 3) > } > } else if (start) { > data = data.slice(start); > } > - // Our implementation of application/newlines. > + > + if (request && request.getHeader("accept") == "application/newlines") { Do we still need this? ISTM you've removed the one place we send that accept header. I'm a little confused by the existing behaviour though, and why it cares about application/newlines given it seems to handle incremental data OK. I'm out of time tonight to dig deeper here though. ::: services/sync/tests/unit/test_collection_getBatched.js:162 (Diff revision 3) > > ok(!response.success); > equal(response.status, 412); > }); > > -add_task(async function test_get_throws() { > +function checkRecordsOrder(records) { seems we might as well keep test_get_throws, if for no better reason than the error semantics? And ISTM that records.length might be zero in the new test, which will still pass. ::: services/sync/tests/unit/xpcshell.ini (Diff revision 3) > [test_resource_ua.js] > [test_syncstoragerequest.js] > > # Generic Sync types. > [test_browserid_identity.js] > -[test_collection_inc_get.js] I think we should keep this test (obviously with major restructuring though) - in particular, the calling of onProgress with partial payloads seems valuable and unique to this test.

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review148708 ::: services/sync/tests/unit/test_bookmark_duping.js:74 (Diff revision 3) > } > > function getServerRecord(collection, id) { > let wbo = collection.get({ full: true, ids: [id] }); > // Whew - lots of json strings inside strings. > - return JSON.parse(JSON.parse(JSON.parse(wbo).payload).ciphertext); > + return JSON.parse(JSON.parse(JSON.parse(JSON.parse(wbo)[0]).payload).ciphertext); I meant to say I don't quite understand this change, and one or 2 other test changes, but that's probably just because I'm not looking hard enough :)

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review148694 > Do we still need this? ISTM you've removed the one place we send that accept header. > > I'm a little confused by the existing behaviour though, and why it cares about application/newlines given it seems to handle incremental data OK. I'm out of time tonight to dig deeper here though. I'd like to keep this, so we can have a behavior consistent with the real server

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review148708 > I meant to say I don't quite understand this change, and one or 2 other test changes, but that's probably just because I'm not looking hard enough :) This line relied on the fact that the test server served 1 record in application/newline mode by default, which is wrong and why I want to keep the changes in comment 32.

Comment on attachment 8866929 [details] (DO NOT REVIEW) - Change getBatched() to return records directly instead of using a callback. https://reviewboard.mozilla.org/r/138456/#review148694 > I think we should keep this test (obviously with major restructuring though) - in particular, the calling of onProgress with partial payloads seems valuable and unique to this test. It seems like this equals testing resource.js, because we don't override _onProgress in Collection.

Comments addressed, TPS works again and event loop spinning patch split in 2.

Rebased with bug 1370985 patch and removed eslint failures.

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review152472 ::: browser/base/content/browser-sync.js:280 (Diff revision 3) > replaceQueryString: true > }); > }, > > - sendTabToDevice(url, clientId, title) { > - Weave.Service.clientsEngine.sendURIToClientForDisplay(url, clientId, title); > + async sendTabToDevice(url, clientId, title) { > + await Weave.Service.clientsEngine.sendURIToClientForDisplay(url, clientId, title); Does this promise resolve with anything a caller might want? If so, possibly `return await` instead. ::: services/common/async.js:242 (Diff revision 3) > + /** > + * A "tight loop" of promises can still lock up the browser for some time. > + * Periodically waiting for a promise returned by this function will solve > + * that. > + * You should call promiseYield every X iterations, X depending on how long > + * does one iteration takes. Maybe mention in this comment that the common use case of calling it every X iterations is handled by jankYielder. ::: services/sync/modules/engines.js:495 (Diff revision 3) > this._declined = new Set(); > this._log = Log.repository.getLogger("Sync.EngineManager"); > this._log.level = Log.Level[Svc.Prefs.get("log.logger.service.engines", "Debug")]; > } > EngineManager.prototype = { > - get(name) { > + async get(name) { Not a fan of these being async just because of initialization... Especially since initialization wasn't lazy before this patch. Can we put off the initialize call until whenever you need to use the engine in an initialized state? Thinking about it a little more, if you really want it to be the way it is now (and possibly even if you change it), we need a separate "initializing" state. As it is, since we set `this.initialized = true;` at the start of `initialize()` (via the super call), if two promise tasks both call `engineManager.get("bookmarks")`, the second will return the bookmarks engine which has `this.initialized == true`, but isn't actually finished initializing. Moving the `this.initialized = true` to the end of `initialize` would fix the issue, but would create the (probably worse) issue of the engine being initialized twice. ::: services/sync/modules/engines.js:1338 (Diff revision 3) > - doApplyBatchAndPersistFailed.call(this); > + await doApplyBatchAndPersistFailed.call(this); > > count.newFailed = this.previousFailed.reduce((count, engine) => { > if (failedInPreviousSync.indexOf(engine) == -1) { > count++; > this._noteApplyNewFailure(); While you're here... if you feel so inclined, you can remove the `_noteApplyNewFailure` and `_noteApplyFailure` methods and callers, since we never override them and very likely never will, since we do telemetry differently than how these expected. ::: services/sync/modules/engines/bookmarks.js:861 (Diff revision 3) > - Async.promiseSpinningly((async () => { > - // Save a backup before clearing out all bookmarks. > + // Save a backup before clearing out all bookmarks. > + try { > await PlacesBackups.create(null, true); > + } catch (ex) { > + this._log.warn("Error while backing up bookmarks, but continuing with wipe", ex); Err, are we certain we shouldn't just let this throw here? Seems risky -- Whatever prevented us from backing up seems like it could prevent us from doing a normal sync, and so we'll just have wiped the users bookmarks with no backup, and then failed to sync them...

****, hit the publish button when I meant to hit edit. In general I think it looks good, but we need to be careful about reentrancy issues like the ones introduced by making get() async.

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review154970 This is looking great - it is r+ except for the engineManager changes (which I think will mean less changes to files outside the sync dir) I'm yet to look at the tests though (not that I expect many issues there though) ::: commit-message-e410a:1 (Diff revision 3) > +Bug 1210296 part 1 - Remove event loop spinning from Sync. r?markh,tcsc,kitcambridge IIRC, there's still some spinning left around observers, right? If so, the commit message should be tweaked (eg, add "most" :) ::: browser/base/content/browser-sync.js:280 (Diff revision 3) > replaceQueryString: true > }); > }, > > - sendTabToDevice(url, clientId, title) { > - Weave.Service.clientsEngine.sendURIToClientForDisplay(url, clientId, title); > + async sendTabToDevice(url, clientId, title) { > + await Weave.Service.clientsEngine.sendURIToClientForDisplay(url, clientId, title); I think we previously semi-agreed that in this case we should return the promise without an await (but yeah, I'm torn in may ways) ::: services/sync/modules/SyncedTabs.jsm:214 (Diff revision 3) > if (!weaveXPCService.ready) { > log.debug("Sync isn't yet ready; assuming tab engine is enabled"); > return true; > } > > - let engine = Weave.Service.engineManager.get("tabs"); > + let engine = Async.promiseSpinningly(Weave.Service.engineManager.get("tabs")); not too keen on this - I think we should be able to arrange for engineManager.get() to be sync after service.ready is true with some additional work. ::: services/sync/modules/bookmark_validator.js:290 (Diff revision 3) > let records = []; > let recordsByGuid = new Map(); > let syncedRoots = SYNCED_ROOTS; > - let yieldCounter = 0; > + let yielder = Async.jankYielder(); > async function traverse(treeNode, synced) { > - if (++yieldCounter % 50 === 0) { > + await yielder.maybeYield(); why not have yielder just be a closure? (Maybe I'll find out myself later :) (Nope, didn't ;) ::: services/sync/modules/collection_validator.js:8 (Diff revision 3) > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > const Cu = Components.utils; > +Cu.import("resource://services-common/async.js"); I don't think you used this import? (But see below - I think you should;) ::: services/sync/modules/collection_validator.js:141 (Diff revision 3) > // problemData: an instance of the class returned by emptyProblemData(), > // clientRecords: Normalized client records > // records: Normalized server records, > // deletedRecords: Array of ids that were marked as deleted by the server. > - compareClientWithServer(clientItems, serverItems) { > - clientItems = clientItems.map(item => this.normalizeClientItem(item)); > + async compareClientWithServer(clientItems, serverItems) { > + clientItems = await Promise.all(clientItems.map(item => this.normalizeClientItem(item))); I had a bad experience before trying to use Promise.all() with many thousand promises re jank. It's a pain, but I think it's work explicit loops with a jankyielder (unless you demonstrate otherwise by testing with a very large collection) ::: services/sync/modules/engines.js:495 (Diff revision 3) > this._declined = new Set(); > this._log = Log.repository.getLogger("Sync.EngineManager"); > this._log.level = Log.Level[Svc.Prefs.get("log.logger.service.engines", "Debug")]; > } > EngineManager.prototype = { > - get(name) { > + async get(name) { I agree with Thom in general. I agree we don't need lazy init, and I think it's fine to have a "contract" that says "the sync service has initialized before calling these functions." Can we have a promiseInitialized or similar on the engine manager, and also one on the service itself that waits for this one, and keep these functions sync? ::: services/sync/modules/engines.js:913 (Diff revision 3) > return; > } > this._previousFailed = val; > Utils.namedTimer(function() { > - Utils.jsonSave("failed/" + this.name, this, val, cb); > + try { > + Async.promiseSpinningly(Utils.jsonSave("failed/" + this.name, this, val)); I don't think it's necessary to spin in the timer function - just .then/.catch? ::: services/sync/modules/engines/bookmarks.js:861 (Diff revision 3) > - Async.promiseSpinningly((async () => { > - // Save a backup before clearing out all bookmarks. > + // Save a backup before clearing out all bookmarks. > + try { > await PlacesBackups.create(null, true); > + } catch (ex) { > + this._log.warn("Error while backing up bookmarks, but continuing with wipe", ex); +1 to what Thom said. ::: services/sync/modules/engines/bookmarks.js:996 (Diff revision 3) > break; > case "bookmarks-restore-success": > this._log.debug("Tracking all items on successful import."); > > this._log.debug("Restore succeeded: wiping server and other clients."); > - this.engine.service.resetClient([this.name]); > + Async.promiseSpinningly(this.engine.service.resetClient([this.name])); I think some of the problem we have with nested event loops is actually creating them in the first place (eg, that can roll menus up etc) - so I think we should have one spinningly for an inline async function ::: services/sync/modules/engines/extension-storage.js:40 (Diff revision 3) > > syncPriority: 10, > allowSkippedRecord: false, > > - _sync() { > - return Async.promiseSpinningly(extensionStorageSync.syncAll()); > + async _sync() { > + return extensionStorageSync.syncAll(); re my previous comment above re "await or return at the end of the function" - there were a number of other examples above where you await'd, but here you return. I don't actually care that much, but it does seem like we should be consistent. (I'm not opening an issue for this, just ranting philosophically ;) ::: services/sync/modules/engines/history.js:236 (Diff revision 3) > }, > > // See bug 468732 for why we use SQL here > _findURLByGUID: function HistStore__findURLByGUID(guid) { > this._urlStm.params.guid = guid; > return Async.querySpinningly(this._urlStm, this._urlCols)[0]; we should get a followup bug to remove querySpinningly too - that will probably be fairly easy after this patch. (And TBH I somewhat skipped reviewing this file, but I will have a closer look next time around :) ::: services/sync/modules/record.js:1013 (Diff revision 3) > throw new Error(`Invalid client/server batch state - client has ${this.batchID}, server has ${responseBatchID}`); > } > > - this.postCallback(response, true); > + await this.postCallback(response, true); > }, > } This file is so much nicer now the getBatched patch landed :) ::: services/sync/modules/service.js:354 (Diff revision 3) > }, > > /** > * Register the built-in engines for certain applications > */ > - _registerEngines: function _registerEngines() { > + _registerEngines() { As above, I think we should make this async, and tweak onStartup above to also be async, with some singleton promiseInitialized or similar function - and .sync() or any other public functions exposed here just awaits for that promise. ::: services/sync/modules/util.js:88 (Diff revision 3) > * > * Optionally pass a function which will be called if an > * exception occurs. > */ > - catch: function Utils_catch(func, exceptionCallback) { > + catch(func, exceptionCallback) { > let thisArg = this; I haven't got to the tests yet, and haven't looked at this stuff closely, but it will be important all these functions do what we think they will ;)

Thank you both for your reviews. I amended my patches.

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review155394 ::: services/sync/modules/engines/bookmarks.js:997 (Diff revisions 3 - 4) > - Async.promiseSpinningly(this.engine.service.resetClient([this.name])); > - Async.promiseSpinningly(this.engine.service.wipeServer([this.name])); > - Async.promiseSpinningly(this.engine.service.clientsEngine.sendCommand( > - "wipeEngine", [this.name], > - null, { reason: "bookmark-restore" })); > + Async.promiseSpinningly(async () => { > + await this.engine.service.resetClient([this.name]); > + await this.engine.service.wipeServer([this.name]); > + await this.engine.service.clientsEngine.sendCommand("wipeEngine", [this.name], > + null, { reason: "bookmark-restore" }); > + }); don't you need to call this async function to get the promise?

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review155398 looking good! ::: browser/base/content/browser-sync.js:283 (Diff revision 4) > switchToTabHavingURI(url, true, { > replaceQueryString: true > }); > }, > > - sendTabToDevice(url, clientId, title) { > + async sendTabToDevice(url, clientId, title) { It looks like the caller of this hasn't changed, so I think you should not make the function async, and just put a trailing .catch on the promise returned by sendURIToClient... ::: services/common/async.js:237 (Diff revision 4) > cb(err || new Error("Promise rejected without explicit error")); > }); > return cb.wait(); > }, > + > + /** The other thing I mentioned before but forgot yesterday is that we still need to get a story for shutdown-exceptions - ie, both these functions should reject once we've seen the shutdown observer. ::: services/common/async.js:254 (Diff revision 4) > + return new Promise(resolve => { > + Services.tm.currentThread.dispatch(resolve, Ci.nsIThread.DISPATCH_NORMAL); > + }); > + }, > + > + jankYielder(yieldEvery = 50) { Please add a comment here for jankYielder, and update the comment above to reflect that it exists (ie, the comment about "you should call promiseYield every X iterations" should just change to refer to the new fn) ::: services/sync/modules/addonsreconciler.js:177 (Diff revision 4) > get addons() { > - this._ensureStateLoaded(); > return this._addons; > }, > > + async initialize() { see comment 23 where I suggested we use an explicit promise here - I think this could be racey - 2 calls to initialize() before this.loadState() resolves would do the wrong thing as they'd both find `this._initialized` as false. ::: services/sync/modules/engines.js:610 (Diff revision 4) > let engine = new engineObject(this.service); > let name = engine.name; > if (name in this._engines) { > this._log.error("Engine '" + name + "' is already registered!"); > } else { > + if (!engine.initialized && engine.initialize) { I think we have a risk of multiple initialization here too - eg, an engine that yields in its initialize() method before calling the base implementation risks being initialized twice, whereas if it calls the base implementation first it risks being considered initialized before it actually is. TBH, I think in this case it's probably best to just drop the .initialized property to avoid misleading future us - IOW, the contract here is "simply don't initialize twice" rather than "it's safe to initialize twice". ::: services/sync/modules/engines.js:828 (Diff revision 4) > > + async initialize() { > + await Engine.prototype.initialize.call(this); > + await this.loadToFetch(); > + await this.loadPreviousFailed(); > + this._log.debug("SyncEngine initialized"); we might as well log the engine name here - just add this.name as a 2nd arg to the log call. ::: services/sync/modules/engines/addons.js:214 (Diff revision 4) > - _syncStartup: function _syncStartup() { > + async _syncStartup() { > // We refresh state before calling parent because syncStartup in the parent > // looks for changed IDs, which is dependent on add-on state being up to > // date. > - this._refreshReconcilerState(); > - > + await this._refreshReconcilerState(); > + return SyncEngine.prototype._syncStartup.call(this); like I said before, I think we should be consistent about whether we "await" at the end of these functions or return a promise (ie, here you return the promise, but just below you await it, then a few lines further down you again return it) ::: services/sync/modules/engines/forms.js:271 (Diff revision 4) > > emptyProblemData() { > return new FormsProblemData(); > } > > getClientItems() { should we add |await| here for consistency? ::: services/sync/modules/engines/history.js:62 (Diff revision 4) > } catch (ex) { } > } > } > notifyHistoryObservers("onBeginUpdateBatch"); > try { > return SyncEngine.prototype._processIncoming.call(this, newitems); I wonder if the semantics here are correct - ISTM we will hit the finally before the returned promise is resolved, which isn't what we want. IOW, I think you want to await on the base impl. ::: services/sync/modules/service.js:1044 (Diff revision 4) > > return reason; > }, > > - sync: function sync(engineNamesToSync) { > + async sync(engineNamesToSync) { > let dateStr = Utils.formatTimestamp(new Date()); shouldn't we wait for the new promiseInitialized here? ::: toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm:50 (Diff revision 4) > // If Sync isn't initialized (either due to lag at startup or due to no user > // being signed in), don't reach in to Weave.Service as that may initialize > // Sync unnecessarily - we'll get an observer notification later when it > // becomes ready and has synced a list of tabs. > if (weaveXPCService.ready) { > - let engine = Weave.Service.engineManager.get("tabs"); > + let engine = await Weave.Service.engineManager.get("tabs"); I think you can revert all changes in this file now that engineManager.get remains sync? ::: toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js:45 (Diff revision 4) > weaveXPCService.ready = true; > > // Configure the singleton engine for a test. > -function configureEngine(clients) { > +async function configureEngine(clients) { > // Configure the instance Sync created. > - let engine = Weave.Service.engineManager.get("tabs"); > + let engine = await Weave.Service.engineManager.get("tabs"); ditto here

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review155398 > The other thing I mentioned before but forgot yesterday is that we still need to get a story for shutdown-exceptions - ie, both these functions should reject once we've seen the shutdown observer. Correct me if I'm wrong, but they already do since promiseYield calls Async.checkAppReady() (which redefines itself to a method that throws once shutdown is observed).

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review155398 > Correct me if I'm wrong, but they already do since promiseYield calls Async.checkAppReady() (which redefines itself to a method that throws once shutdown is observed). ah, yes, sorry about that.

Patch rebased.

Weekly rebase

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review158198 This is looking good, thanks so much for slogging through this! I skimmed over some of the mechanical changes, but I'll read through them again after a break, and see if I missed anything. r+ because I doubt we'll catch all the edge cases in review, and I'd like to get this landed and tested soon. ::: browser/base/content/browser-sync.js:286 (Diff revision 7) > triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(), > }); > }, > > sendTabToDevice(url, clientId, title) { > - Weave.Service.clientsEngine.sendURIToClientForDisplay(url, clientId, title); > + Weave.Service.clientsEngine.sendURIToClientForDisplay(url, clientId, title).catch(e => { It seems unfortunate that this can fail without feedback, but there's not much the user can do if we fail to queue the command...so this is probably OK. ::: services/common/async.js:240 (Diff revision 7) > + /** > + * A "tight loop" of promises can still lock up the browser for some time. > + * Periodically waiting for a promise returned by this function will solve > + * that. > + * You should probably not use this method directly and instead use jankYielder > + * bellow. Micro-nit: below ::: services/sync/modules/bookmark_repair.js:183 (Diff revision 7) > return fixableProblems.reduce((numProblems, problemLabel) => { > return numProblems + validationInfo.problems[problemLabel].length; > }, 0); > } > > - tryServerOnlyRepairs(validationInfo) { > + async tryServerOnlyRepairs(validationInfo) { Hmm, do `tryServerOnlyRepairs` and `startRepairs` need to be async? ::: services/sync/modules/bookmark_repair.js:694 (Diff revision 7) > if (data != "bookmarks") { > return; > } > Svc.Obs.remove("weave:engine:sync:uploaded", this.onUploaded, this); > log.debug(`bookmarks engine has uploaded stuff - creating a repair response`); > - this._finishRepair(); > + Async.promiseSpinningly(this._finishRepair()); I'm curious how `.then(...)` handlers will interact with event loop spinning. Specifically, what happens in the case of `Promise.resolve().then(_ => { Services.obs.notifyObservers(...); return anotherPromiseReturningFunction(); })`, where one of the observers spins the event loop to wait on another promise chain? That's a simplified example; the `notifyObservers` call might be several levels removed from the handler. In theory, that should be OK, but I'm wondering if there are gotchas that we need to keep in mind. ::: services/sync/modules/bookmark_validator.js:10 (Diff revision 7) > "use strict"; > > const { interfaces: Ci, utils: Cu } = Components; > > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/Timer.jsm"); I think you can remove the `Timer.jsm` import, now that `setTimeout` is gone? ::: services/sync/modules/collection_validator.js:8 (Diff revision 7) > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > const Cu = Components.utils; > +Cu.import("resource://services-common/async.js"); Could this be a lazy import, here and elsewhere? ::: services/sync/modules/engines.js:660 (Diff revision 7) > this._log = Log.repository.getLogger("Sync.Engine." + this.Name); > let level = Svc.Prefs.get("log.logger.engine." + this.name, "Debug"); > this._log.level = Log.Level[level]; > > this._modified = this.emptyChangeset(); > this._tracker; // initialize tracker to load previously changed IDs While you're here, can you remove this, too, or will that cause issues? I don't think we load previously changed IDs in the `Tracker` constructor anymore... ::: services/sync/modules/engines.js:879 (Diff revision 7) > return; > } > this._toFetch = val; > Utils.namedTimer(function() { > - Utils.jsonSave("toFetch/" + this.name, this, val, cb); > + try { > + Async.promiseSpinningly(Utils.jsonSave("toFetch/" + this.name, this, val)); It looks like this happened in the background before, without waiting for `jsonSave` to finish. `previousFailed` also saves in the background. I think your change seems closer to the desired behavior (though this can still be interrupted by shutdown, since we don't register a blocker), but something to note. ::: services/sync/modules/engines.js:1714 (Diff revision 7) > counts.sent += pendingSent; > pendingSent = 0; > } > }); > > - let pendingWeakReupload = this.buildWeakReuploadMap(this._needWeakReupload); > + let pendingWeakReupload = await this.buildWeakReuploadMap(this._needWeakReupload); It's a bit unfortunate that we need to periodically yield in `buildWeakReuploadMap`, too, but so it goes. ::: services/sync/modules/engines/forms.js:60 (Diff revision 7) > - > - _updateSpinningly(changes) { > if (!FormHistory.enabled) { > - return; // update isn't going to do anything. > + return Promise.resolve(); // update isn't going to do anything. > } > - let cb = Async.makeSpinningCallback(); > + return new Promise(resolve => { I don't really have a preference one way or another, but, since this is already `async`, you could replace `return` with `await` here, and drop the `Promise.resolve()` above. ::: services/sync/modules/engines/history.js:307 (Diff revision 7) > this._asyncHistory.updatePlaces(records, updatePlacesCallback); > })); > } > > - // Since `failed` is updated asynchronously and this function is > - // synchronous, we need to spin-wait until we are sure that all > + // failed is updated asynchronously, hence the await on blockers. > + await Promise.all(blockers); `History.jsm` has a cleaner `insertMany` wrapper around `updatePlaces` that returns a promise. I filed bug 1377944 as a follow-up. ::: services/sync/modules/engines/passwords.js:121 (Diff revision 7) > return null; > } > > let logins = Services.logins.findLogins({}, login.hostname, login.formSubmitURL, login.httpRealm); > > - this._store._sleep(0); // Yield back to main thread after synchronous operation. > + await Async.promiseYield(); // Yield back to main thread after synchronous operation. We seem to be inconsistent about where we yield in this engine. For example, we yield after `Services.logins.findLogins` here, but not in or after `getAllIDs`, `changeItemID`, `create`, `update`, or `remove`, even though those also call `Services.logins` methods. IIUC, though, we use a JSON backend for login storage on Desktop, so none of these operations should perform sync I/O...maybe we don't need to yield at all? ::: services/sync/modules/policies.js:362 (Diff revision 7) > } > }, > > updateGlobalScore() { > - let engines = [this.service.clientsEngine].concat(this.service.engineManager.getEnabled()); > + let enabled = this.service.engineManager.getEnabled(); > + let engines = [this.service.clientsEngine].concat(enabled); This change doesn't seem necessary? ::: services/sync/modules/service.js:375 (Diff revision 7) > } > > - this.clientsEngine = new ClientEngine(this); > + let clientsEngine = new ClientEngine(this); > + // Ideally clientsEngine should not exist > + // (or be a promise that calls initialize() before returning the engine) > + Async.promiseSpinningly(clientsEngine.initialize()); Why do we spin here, but use `await this.engineManager.register` in the loop below? ::: services/sync/modules/service.js:1257 (Diff revision 7) > if (!engines) { > // Clear out any service data > - this.resetService(); > + await this.resetService(); > > - engines = [this.clientsEngine].concat(this.engineManager.getAll()); > + let allEngines = this.engineManager.getAll(); > + engines = [this.clientsEngine].concat(allEngines); Revert. ::: services/sync/modules/service.js:1397 (Diff revision 7) > Svc.Obs.notify("weave:telemetry:event", { object, method, value, extra }); > }, > }; > > this.Service = new Sync11Service(); > -this.Service.onStartup(); > +this.Service.promiseInitialized = new Promise(resolve => { Nit: Define `promiseInitialized` as a lazy getter on the prototype, like you did for `AddonsReconciler#ensureStateLoaded`? ::: services/sync/modules/stages/enginesync.js:49 (Diff revision 7) > } > > // this is a purposeful abort rather than a failure, so don't set > // any status bits > reason = "Can't sync: " + reason; > - this.onComplete(new Error("Can't sync: " + reason)); > + throw new Error("Can't sync: " + reason); Nit: `throw new Error(reason)` ::: services/sync/modules/stages/enginesync.js:144 (Diff revision 7) > // If the engines to sync has been specified, we sync in the order specified. > let enginesToSync; > if (allowEnginesHint && engineNamesToSync) { > this._log.info("Syncing specified engines", engineNamesToSync); > - enginesToSync = engineManager.get(engineNamesToSync).filter(e => e.enabled); > + let engines = engineManager.get(engineNamesToSync); > + enginesToSync = engines.filter(e => e.enabled); Revert.

Florian, does this sound OK to you? I asked :mconley about promises and event loop spinning in #fx-team, and he pointed me to you. (In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #61) > I'm curious how `.then(...)` handlers will interact with event loop spinning. > > Specifically, what happens in the case of `Promise.resolve().then(_ => { > Services.obs.notifyObservers(...); return anotherPromiseReturningFunction(); > })`, where one of the observers spins the event loop to wait on another > promise chain? > > That's a simplified example; the `notifyObservers` call might be several > levels removed from the handler. > > In theory, that should be OK, but I'm wondering if there are gotchas that we > need to keep in mind.

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review159232 nice work - I'll look at the tests tomorrow. ::: services/common/async.js:260 (Diff revision 7) > + let iterations = 0; > + return async () => { > + if (++iterations % yieldEvery === 0) { > + await Async.promiseYield(); > + } > + } Sorry to keep bringing this up, but I think regressing jank is a significant risk with this patch - I could imagine a sync that has many jankYielder calls, but with none of them quite hitting 50 iterations, and thus not noticing the shutdown. I think we can probably make a simple (but arguably ugly) improvement here - how about having the observer in checkAppReady also overwrite both promiseYield and jankYielder, in the same way it overwrites checkAppReady? That would also mean you could remove the checkAppReady call from promiseYield. If so, we should also add comments pointing to that function and indicating it may be overridden. ::: services/sync/modules/SyncedTabs.jsm:15 (Diff revision 7) > const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components; > > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/Log.jsm"); > +Cu.import("resource://services-common/async.js"); I don't think you need this import? ::: services/sync/modules/addonsreconciler.js:517 (Diff revision 7) > }, > > /** > * Obtain the add-on state record for an add-on by Sync GUID. > * > - * If the add-on could not be found, returns null. > + * If the add-on could not be found, returns a promise that resolves to null. This function doesn't return a promise. Also, we might as well stich with the @return syntax seeing we still have @param (IOW, I think this entire comment change should be reverted) ::: services/sync/modules/engines.js:600 (Diff revision 7) > * Engine object used to get an instance of the engine > * @return The engine object if anything failed > */ > - register(engineObject) { > + async register(engineObject) { > if (Array.isArray(engineObject)) { > engineObject.map(this.register, this); I don't think this will do the right thing (ie, will not wait for each .register call). I doubt it is used - maybe you could just delete it? ::: services/sync/modules/engines.js:879 (Diff revision 7) > return; > } > this._toFetch = val; > Utils.namedTimer(function() { > - Utils.jsonSave("toFetch/" + this.name, this, val, cb); > + try { > + Async.promiseSpinningly(Utils.jsonSave("toFetch/" + this.name, this, val)); I think this is still in the background courtesy of the namedTimer - but yeah, the shutdown blocker is a good point - we really should do bug 1366067 soonish ::: services/sync/modules/engines/addons.js (Diff revision 7) > /** > * Synchronously obtain an add-on from its public ID. > * > * @param id > * Add-on ID > - * @return Addon or undefined if not found we might as well keep the @return comment ::: services/sync/modules/engines/passwords.js:91 (Diff revision 7) > } > } > if (ids.length) { > let coll = new Collection(this.engineURL, null, this.service); > coll.ids = ids; > let ret = coll.delete(); need to await here. ::: services/sync/modules/engines/passwords.js:121 (Diff revision 7) > return null; > } > > let logins = Services.logins.findLogins({}, login.hostname, login.formSubmitURL, login.httpRealm); > > - this._store._sleep(0); // Yield back to main thread after synchronous operation. > + await Async.promiseYield(); // Yield back to main thread after synchronous operation. to be fair though, this patch isn't introducing that - maybe a followup? ::: services/sync/modules/service.js:375 (Diff revision 7) > } > > - this.clientsEngine = new ClientEngine(this); > + let clientsEngine = new ClientEngine(this); > + // Ideally clientsEngine should not exist > + // (or be a promise that calls initialize() before returning the engine) > + Async.promiseSpinningly(clientsEngine.initialize()); and the comment doesn't make sense to me either. ::: services/sync/modules/service.js:1397 (Diff revision 7) > Svc.Obs.notify("weave:telemetry:event", { object, method, value, extra }); > }, > }; > > this.Service = new Sync11Service(); > -this.Service.onStartup(); > +this.Service.promiseInitialized = new Promise(resolve => { I'm not sure we should do that actually - IIRC, weave.js assumes that the .ready observer (and thus the ensure ready state) will be sent as a side-effect of importing this module. ::: services/sync/modules/stages/declined.js:31 (Diff revision 7) > this._log.level = Log.Level[new Preferences(PREFS_BRANCH).get("log.logger.declined")]; > > this.service = service; > } > this.DeclinedEngines.prototype = { > - updateDeclined(meta, engineManager = this.service.engineManager) { > + async updateDeclined(meta, engineManager = this.service.engineManager) { why is this async?

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review159232 > Sorry to keep bringing this up, but I think regressing jank is a significant risk with this patch - I could imagine a sync that has many jankYielder calls, but with none of them quite hitting 50 iterations, and thus not noticing the shutdown. > > I think we can probably make a simple (but arguably ugly) improvement here - how about having the observer in checkAppReady also overwrite both promiseYield and jankYielder, in the same way it overwrites checkAppReady? That would also mean you could remove the checkAppReady call from promiseYield. If so, we should also add comments pointing to that function and indicating it may be overridden. This will also mean that we will only check for shutdown on the first loop iteration or every 50 iterations since jankYielder returns an anonymous function. Is that okay with you if we checkAppReady in every call of the returned closure?

(In reply to Edouard Oger [:eoger] from comment #64) > Is that okay with you if we checkAppReady in every call of the returned > closure? Yeah, that sounds fine.

Comment on attachment 8874620 [details] Bug 1210296 part 2 - Update tests. https://reviewboard.mozilla.org/r/145964/#review159376 Nice work, thanks - r=me if the services/sync/tests/unit/test_service_startup.js changes can be reverted. Can you please push this to try, and do many many retries of xpcshell tests to try and pre-empt any new oranges introduced (eg, forgetting to await a promie somewhere seems likely to be orange - timing might mean it works most of the time but not always) ::: services/sync/modules-testing/fakeservices.js:49 (Diff revision 7) > obj = JSON.parse(json); > } > - if (cb) { > - cb.call(that, obj); > - } > return Promise.resolve(obj); can't you just return obj here? ::: services/sync/tests/unit/head_helpers.js:24 (Diff revision 7) > Cu.import("resource://gre/modules/ObjectUtils.jsm"); > > +add_task(async function head_setup() { > + // If a test imports Service, make sure is initialized first. > + if (this.Service) { > + await Service.promiseInitialized; you should probably use this.Service. here too for consistency. ::: services/sync/tests/unit/head_helpers.js:394 (Diff revision 7) > }); > } > > async function wait_for_ping(callback, allowErrorPings, getFullPing = false) { > let pingsPromise = wait_for_pings(1); > - callback(); > + await Promise.resolve(callback()); do you need the Promise.resolve() here? ie, can't you just await for the result of the callback, which should do the right thing whether it returns a promise or not? ::: services/sync/tests/unit/test_addons_reconciler.js:139 (Diff revision 7) > - do_check_eq(null, error); > - do_check_false(loaded); > + do_check_false(loaded); > > - do_check_eq("object", typeof(reconciler.addons)); > + do_check_eq("object", typeof(reconciler.addons)); > - do_check_eq(1, Object.keys(reconciler.addons).length); > - do_check_eq(1, reconciler._changes.length); > + do_check_eq(0, Object.keys(reconciler.addons).length); > + do_check_eq(0, reconciler._changes.length); As discussed on IRC, the old test seems somewhat broken here - we are explicitly testing we do *not* load the future-version state file. ::: services/sync/tests/unit/test_bookmark_order.js:48 (Diff revision 7) > }, > [engine.name]: {}, > }); > } > > +add_task(async function setup() { I don't really care about this, but these could all be in the top-level scope instead of in a test function. ::: services/sync/tests/unit/test_bookmark_repair.js:34 (Diff revision 7) > > +let clientsEngine; > +let bookmarksEngine; > +var recordedEvents = []; > + > +add_task(async function setup() { ditto here - although here I think it is worse, as this setup function is overriding Services.recordTelemetryEvent which is used by future tests, so IMO it is clearer if it is at the top leve. ::: services/sync/tests/unit/test_bookmark_repair_responder.js:40 (Diff revision 7) > - Svc.Prefs.reset("engine.bookmarks.validation.enabled"); > // clear keys so when each test finds a different server it accepts its keys. > Service.collectionKeys.clear(); > } > > +let bookmarksEngine; ditto here - leave these at the top-level. ::: services/sync/tests/unit/test_errorhandler_1.js:412 (Diff revision 7) > }); > await promiseObserved; > > do_check_eq(Status.sync, CREDENTIALS_CHANGED); > // If we clean this tick, telemetry won't get the right error > await promiseNextTick(); You could consider replacing all promiseNextTick with promiseYield (but I don't really care) ::: services/sync/tests/unit/test_fxa_node_reassignment.js:148 (Diff revision 7) > // Make absolutely sure that any event listeners are done with their work > // before we proceed. > waitForZeroTimer(function() { > _("Second sync nextTick."); > do_check_eq(numTokenRequests, numTokenRequestsBefore + 1, "fetched a new token"); > - Service.startOver(); > + Async.promiseSpinningly(Service.startOver()); let's make this a promise chain? ::: services/sync/tests/unit/test_interval_triggers.js:374 (Diff revision 7) > do_check_eq(scheduler.syncInterval, scheduler.activeInterval); > do_check_true(scheduler.syncTimer.delay <= scheduler.activeInterval); > > scheduler.scheduleNextSync = scheduler._scheduleNextSync; > Svc.Obs.remove("weave:service:sync:finish", onSyncFinish); > - Service.startOver(); > + Async.promiseSpinningly(Service.startOver()); ditto here ::: services/sync/tests/unit/test_node_reassignment.js:126 (Diff revision 7) > - // Make absolutely sure that any event listeners are done with their work > + // Make absolutely sure that any event listeners are done with their work > - // before we proceed. > + // before we proceed. > - waitForZeroTimer(function() { > + waitForZeroTimer(function() { > - _("Second sync nextTick."); > + _("Second sync nextTick."); > - do_check_eq(getTokenCount, 1); > + do_check_eq(getTokenCount, 1); > - Service.startOver(); > + Async.promiseSpinningly(Service.startOver()); and here. ::: services/sync/tests/unit/test_node_reassignment.js:358 (Diff revision 7) > // before we proceed. > waitForZeroTimer(function() { > _("Third sync nextTick."); > do_check_false(getReassigned()); > do_check_eq(getTokenCount, 2); > - Service.startOver(); > + Async.promiseSpinningly(Service.startOver()); and here (and anywhere else below :) ::: services/sync/tests/unit/test_service_startup.js (Diff revision 7) > Cu.import("resource://services-sync/service.js"); > > _("Service is enabled."); > do_check_eq(Service.enabled, true); > > - _("Engines are registered."); why the removal of these checks? ::: services/sync/tests/unit/test_service_sync_remoteSetup.js:120 (Diff revision 7) > Service.recordManager.del(Service.metaURL); > _("Checking that remoteSetup recovers on 404 on first get /meta/global."); > - do_check_true(Service._remoteSetup()); > + do_check_true((await Service._remoteSetup())); > mock.restore(); > > let makeOutdatedMeta = () => { can't you make this async and await for it where it is called below?

Comment on attachment 8874620 [details] Bug 1210296 part 2 - Update tests. https://reviewboard.mozilla.org/r/145964/#review159376 > ditto here - leave these at the top-level. We can't really touch Service until head_setup() which waits for promiseInitialized has been executed. I moved back what I could into the top level scope of these files.

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #62) > > Specifically, what happens in the case of `Promise.resolve().then(_ => { > > Services.obs.notifyObservers(...); return anotherPromiseReturningFunction(); > > })`, where one of the observers spins the event loop to wait on another > > promise chain? FWIW, I don't think we've actually changed the semantics significantly here - we already have observers spinning nested event loops (in the case you cited, it is currently via the client's engine doing a Utils.jsonSave() when we queue a command, which arguably isn't ideal, but is what happens now.) We have added promises to this mix, but I'm not sure that's particularly relevant, although I do expect we might get some strangeness with "async stacks".

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #62) > Florian, does this sound OK to you? I asked :mconley about promises and > event loop spinning in #fx-team, and he pointed me to you. > > (In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #61) > > I'm curious how `.then(...)` handlers will interact with event loop spinning. > > > > Specifically, what happens in the case of `Promise.resolve().then(_ => { > > Services.obs.notifyObservers(...); return anotherPromiseReturningFunction(); > > })`, where one of the observers spins the event loop to wait on another > > promise chain? > > > > That's a simplified example; the `notifyObservers` call might be several > > levels removed from the handler. > > > > In theory, that should be OK, but I'm wondering if there are gotchas that we > > need to keep in mind. My current understanding is that 'then' callbacks on resolved promises are micro-tasks that are executed before attempting to take events from the event queue. I can't say for sure how the code would behave in the example case you gave.

Comment on attachment 8872819 [details] Bug 1210296 part 1 - Remove most event loop spinning from Sync. https://reviewboard.mozilla.org/r/144336/#review159916 The issues I brought up last time have been addressed, and I don't see any logic flaws (although I suspect for these sorts of changes the danger is more in the code that doesn't appear in the diff than the code that does). Or at least, none introduced by these changes. I'm very concerned about timing assumptions being invalidated by these changes, but I don't think there's anything that could be done to avoid those. Please make sure all TPS tests pass with these changes, though. If any seem to fail w/o your changes, ping me in IRC and I can try to get you a workaround (As of the start of the week, they should all pass). Anyway, the only concrete issues I have are nits. ::: services/sync/modules/collection_validator.js:102 (Diff revision 9) > return true; > } > > // Turn the client item into something that can be compared with the server item, > // and is also safe to mutate. > - normalizeClientItem(item) { > + async normalizeClientItem(item) { I think this doesn't need to be async, does it? None of the implementations are async. Not that it really hurts... ::: services/sync/modules/collection_validator.js:144 (Diff revision 9) > // problemData: an instance of the class returned by emptyProblemData(), > // clientRecords: Normalized client records > // records: Normalized server records, > // deletedRecords: Array of ids that were marked as deleted by the server. > - compareClientWithServer(clientItems, serverItems) { > - clientItems = clientItems.map(item => this.normalizeClientItem(item)); > + async compareClientWithServer(clientItems, serverItems) { > + let maybeYield = Async.jankYielder(); Is there a reason for the jank yielder? Or just being safe. We already limit the number of records we use for validation, and we never even run validation outside of cases like aboutsync. It doesn't really matter, but might simplify the code some. ::: services/sync/modules/engines.js:719 (Diff revision 9) > > if (!this._sync) { > throw "engine does not implement _sync method"; > } > > - this._notify("sync", this.name, this._sync)(); > + return this._notify("sync", this.name, this._sync)(); I think `return await` might be worthwhile here (and in a couple other places) to keep this function on the stack trace. Not picky though, feel free to drop if you disagree (or if we have made a style decision to avoid doing that or something) ::: services/sync/modules/engines.js:1818 (Diff revision 9) > let key = this.service.collectionKeys.keyForCollection(this.name); > > // Any failure fetching/decrypting will just result in false > try { > this._log.trace("Trying to decrypt a record from the server.."); > - let json = Async.promiseSpinningly(test.get()).obj[0]; > + let json = (await test.get()).obj[0]; Even without the spinning this is still gross :( (Not worth addressing here though) ::: services/sync/modules/engines/clients.js:755 (Diff revision 9) > * clients. > * @param flowID > * A unique identifier used to track success for this operation across > * devices. > */ > - sendCommand(command, args, clientId = null, telemetryExtra = {}) { > + async sendCommand(command, args, clientId = null, telemetryExtra = {}) { Please adjust the doc comment to explain why this function is async, since it's not 100% obvious from the name why it would be -- Both kit and I spent time digging into it when reviewing during the workweek. Might be worth adjusting the comments on other newly async functions in this file too.

Comment on attachment 8874620 [details] Bug 1210296 part 2 - Update tests. https://reviewboard.mozilla.org/r/145964/#review159926 I'm not picky about test changes as long as they still pass, and still actually test what they want to (and afaict these should). For the former, I agree with markh about running a large number of try runs to catch oranges in advance. ::: services/sync/tests/unit/head_http_server.js:278 (Diff revision 8) > * Removes an object entirely from the collection. > * > * @param id > * (string) ID to remove. > */ > - remove: function remove(id) { > + async remove(id) { Why is this async? Looks like overzealous text editor find/replace.

Comment on attachment 8874620 [details] Bug 1210296 part 2 - Update tests. https://reviewboard.mozilla.org/r/145964/#review159194 Rubber stamp r+, with some nits that you're welcome to ignore. Thanks for sticking with this patch! ::: services/sync/tests/unit/head_errorhandler_common.js:36 (Diff revision 6) > syncID: Service.syncID, > storageVersion: STORAGE_VERSION, > engines: {clients: {version: Service.clientsEngine.version, > syncID: Service.clientsEngine.syncID}, > - catapult: {version: Service.engineManager.get("catapult").version, > - syncID: Service.engineManager.get("catapult").syncID}} > + catapult: {version: catapultEngine.version, > + syncID: catapultEngine.syncID}} Revert? ::: services/sync/tests/unit/head_helpers.js:22 (Diff revision 6) > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/PlacesUtils.jsm"); > Cu.import("resource://gre/modules/ObjectUtils.jsm"); > > +add_task(async function head_setup() { > + // If a test imports Service, make sure is initialized first. Micro-nit: "make sure it is initialized first" ::: services/sync/tests/unit/head_helpers.js:394 (Diff revision 6) > }); > } > > async function wait_for_ping(callback, allowErrorPings, getFullPing = false) { > let pingsPromise = wait_for_pings(1); > - callback(); > + await Promise.resolve(callback()); Nit: This gave me pause. Let's use `await callback()`, unless we decided to intentionally use `await Promise.resolve(...)` as a metter of style. ::: services/sync/tests/unit/head_helpers.js:21 (Diff revision 9) > Cu.import("resource://services-sync/util.js"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/PlacesUtils.jsm"); > Cu.import("resource://gre/modules/ObjectUtils.jsm"); > > +add_task(async function head_setup() { This feels a bit magical to me, but it's a clever shortcut, and avoids threading `await Service.promiseInitialized` throughout all the tests... ::: services/sync/tests/unit/head_http_server.js:278 (Diff revision 9) > * Removes an object entirely from the collection. > * > * @param id > * (string) ID to remove. > */ > - remove: function remove(id) { > + remove(id) { Revert? ::: services/sync/tests/unit/test_bookmark_duping.js:20 (Diff revision 9) > +let engine; > +let store; > > -Service.engineManager.register(BookmarksEngine); > - > -const engine = new BookmarksEngine(Service); > +add_task(async function setup() { > + initTestLogging("Trace"); > + await Service.engineManager.register(BookmarksEngine); I can't find the bug, but I think we talked about how these `register` calls will register a second instance of the engine, and wreak havoc with the scheduler. While you're here, do you think it makes sense to replace these calls with `Service.engineManager.get("...")` for the built-in engines? I'm also fine with following up later, since you've been waiting to land this for a while, and this isn't something that we should block on. ::: services/sync/tests/unit/test_bookmark_duping.js:44 (Diff revision 9) > Svc.Obs.notify("weave:engine:stop-tracking"); > let promiseStartOver = promiseOneObserver("weave:service:start-over:finish"); > - Service.startOver(); > + await Service.startOver(); > await promiseStartOver; > await promiseStopServer(server); > await bms.eraseEverything(); It occurs to me we duplicate this cleanup logic in a lot of places. Let's get a follow-up mentored bug on file to clean some of this up. ::: services/sync/tests/unit/test_bookmark_smart_bookmarks.js:14 (Diff revision 9) > Cu.import("resource://testing-common/services/sync/utils.js"); > > const SMART_BOOKMARKS_ANNO = "Places/SmartBookmark"; > -var IOService = Cc["@mozilla.org/network/io-service;1"] > +const IOService = Cc["@mozilla.org/network/io-service;1"] > .getService(Ci.nsIIOService); > ("http://www.mozilla.com", null, null); What's this? :-) ::: services/sync/tests/unit/test_bookmark_tracker.js:1669 (Diff revision 9) > let fx_id = PlacesUtils.bookmarks.insertBookmark( > folder2_id, > Utils.makeURI("http://getfirefox.com"), > PlacesUtils.bookmarks.DEFAULT_INDEX, > "Get Firefox!"); > - let fx_guid = engine._store.GUIDForId(fx_id); > + let fx_guid = await engine._store.GUIDForId(fx_id); We can probably ditch `GUIDForId` entirely in a follow-up. ::: services/sync/tests/unit/test_clients_engine.js:52 (Diff revision 9) > let allIDs = new Set(actual.map(elt => elt.flowID).filter(fid => !!fid)); > equal(allIDs.size, actual.length, "all items have unique IDs"); > } > > -function cleanup() { > +add_task(async function setup() { > + engine = Service.clientsEngine Missing `;`, but why is this split out? Do we need to call `await engine.initialize()`? ::: services/sync/tests/unit/test_clients_engine.js:862 (Diff revision 9) > > let remoteId = Utils.makeGUID(); > let rec = new ClientsRec("clients", remoteId); > rec.name = "remote"; > - store.create(rec); > - store.createRecord(remoteId, "clients"); > + await store.create(rec); > + await store.createRecord(remoteId, "clients"); I don't understand why we're calling `createRecord` immediately after `create`. For its side effects? (Feel free to leave this, since it was there before...but it seems fishy).

Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c25b27ba5335 part 1 - Remove most event loop spinning from Sync. r=kitcambridge,markh,tcsc https://hg.mozilla.org/integration/autoland/rev/e4fe9862f23d part 2 - Update tests. r=kitcambridge,markh,tcsc

https://hg.mozilla.org/mozilla-central/rev/c25b27ba5335 https://hg.mozilla.org/mozilla-central/rev/e4fe9862f23d