Services/Sync/WEP/116
Contents
Getting rid of Sync.js
Current situation
We use Sync.js's magic to make certain async calls appear sync. This makes async code beautiful to read and write because it looks like any other sync code. Currently this technique is used in:
- Resource._request()
- Utils.queryAsync()
- (possibly) threaded crypto
Problem
Apparently spinning the event loop is bad. <insert sdwilsh here>
Proposed solution
Everything takes a callback as its last argument. Callbacks always take an error as their first argument. If null/undefined, there wasn't an error. The second argument is the return value, if needed.
FWIW, this is the convention node.js follows. This convention would also allow to develop utilities for syntactic sugar (async chain, map, ...) or perhaps even reuse one of the popular libraries out there.
This is a massive undertaking (see below), so until we can get rid of Sync.js completely, let's divide and conquer: The codebase is converted to async calls step by step, from the inside out. Private helpers are turned async without backwards compatibility. The more public API methods are made async and renamed to methodCb while we provide a sync version of it under the old name for backwards compatibility (using Sync.js).
Task overview
Things that use Resource:
| Method |
Risk |
Used where (big picture) |
Comment |
|---|---|---|---|
| Records.import(), Records.get() | low | SyncEngine.sync(), Service.sync() |
|
| PubKeys.uploadKeypair() | low | Service.sync() | |
| SyncEngine.sync() | low | Service.sync() |
|
| SyncEngine.wipeServer() | low | SyncEngine.sync(), Service.sync() | |
| Service.login() |
high |
UI, returns true/false |
May change semantics of login() if we get rid of "connect"/"disconnect" |
| Service.verifyLogin() |
high |
UI, returns true/false |
|
| Service.changePassword() |
medium |
UI (generic change dialog), returns true/false |
Only called in one place, easily changed |
| Service.changePassphrase() |
medium |
UI (generic change dialog) |
Only called in one place, easily changed |
| Service.checkAccount() |
medium |
UI (wizard), returns true/false |
Only called in two places, easily changed |
| Service._doHeartbeat() |
low |
Service._scheduleHeartbeat(), Service._scheduleNextSync() |
Triggered by observer notifications and timers, always run async anyway |
| Service.sync() |
low |
UI code and timers |
Always run async anyway (at least it should be!) |
| Service.wipeServer(), Service.wipeRemote() |
low |
Service.sync(), Service.changePassphrase(), Service._updateKeysToUTF8Passphrase() |
|
| Service.getCollectionUsage(), Service.getQuota() | low |
UI (quota dialog) |
Already called async |
| Service._handleResource401() |
low |
"weave:resource:status:401" observer notification |
Need to pass additional callback within the notification subject |
Things that use Utils.queryAsync():
| API Method |
Comment |
|---|---|
| BookmarksStore.update() | |
| FormStore.getAllIDs(), FormStore.createRecord(), FormStore.remove(), FormStore.itemExists(), FormStore.changeItemID() | all via FormTracker |
| FormTracker.trackEntry() | via FormTracker |
| FormEngine._findDupe() | via FormTracker |
| HistoryStore._haveTempTables, HistoryStore.update(), HistoryStore.createRecord(), HistoryStore.changeItemID(), HistoryStore.remove(), HistoryStore.itemExists(), HistoryStore.getAllIDs() | |
From this it's pretty clear that the store interface needs to become async. It's probably going to be ok to do this without backwards compatibility as the store is only in during SyncEngine.sync() and its helpers. Unless of course we care about third party implementations.
We might need to turn places annotations as returned by Utils.anno() into async DB queries as well (see bug 584927), at which point the Bookmarks and History trackers would be using Utils.queryAsync() too. However, since trackers are often invoked async anyway (as observers for places/satchel/... notifications), we could just as easily make that fully async.
Things that use Svc.Crypto:
| API Method |
Comment |
|---|---|
| SyncEngine.sync() | |
| Service.onStartup() | Triggered by event handler, can run async anyway. |
| Service._verifyPassphrase() | Only used in Service.verifyLogin() |
| PubKeys.createKeypair() | Only used in Service.sync() |
| CrytoWrapper.encrypt() | Only used in SyncEngine.sync() |
| CryptoMeta.addUnwrappedKey() | Only used in SyncEngine.sync() |
Making crypto async will touch no place that the Resource async work wouldn't touch.
