Services/Sync/WEP/116

From MozillaWiki
< Services‎ | Sync‎ | WEP
Jump to: navigation, search

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.