496297 - Switch yield/self to Sync

Status: RESOLVED FIXED

(Cloud Services :: General, defect)

Description

[Ed Lee :Mardak]

Attachment: resource.js _request

We can use jsmodules' Sync code to call async. functions synchronously.

Attached is an example of changing 3 of resource.js' _request method's yield self.cb to some Sync. (The only one left is let self = yield)

+    let [chanOpen, chanCb] = Sync.withCb(channel.asyncOpen, channel);
Potentially this could be Sync.withCb("asyncOpen", channel)

+      Sync(this.filterUpload, this)();
And Sync("filterUpload", this)();

This patch seems to work for logging in and syncing data.. simple enough of a change, but I haven't changed the call sites..

Comment 1

[Ed Lee :Mardak]

Attachment: fix get/put/post/delete call sites to get login + sync working

De-self=yield-ify _request and convert get/put/post/delete to not take an async callback.. (should those be left async?)

The diff attached only fixes enough call sites to get login and sync working for me..

Comment 2

[Ed Lee :Mardak]

Attachment: fix call sites to get erase server + initial sync working

More call sites fixed when using erase server button then followed by the automatic "initial sync" (and key generation).

I'm using a helper function to record unique stack frames for each of the get/put/post/delete call sites:

new Error().stack.split("\n")[3]


I suppose I could just search statically through the code for the call sites instead of using this dynamic approach...

Comment 3

[Ed Lee :Mardak]

I've created a branch for this Sync conversion:
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/

I'll probably keep converting call sites dynamically instead of statically with grep, so it makes sense to leave this as a branch for now as some code paths aren't fixed yet. (Anybody can join in! ;))

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/a09b2150edbf
Add ext/Sync.js to do sync-async 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/5606f1453adc
Switch Resource._request to Sync. (ChannelListener, filterUpload/Download) 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/78b9943d04d9
Change Resource.get() to be sync (no callback) and fix up call sites used for login + sync now. 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/6697a0bf2e2b
Fix Resource.* call sites used for wiping the server and initial sync (+ key gen upload)

Comment 4

[Ed Lee :Mardak]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/eec19ed7a94c
Change SyncEngine._reconcile to not be async/yield. 

Should HistEngine._reconcile just use the base SyncEngine._reconcile and instead have recordLike return false?

Comment 5

[Ed Lee :Mardak]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/3d8caf76982e
RecordMgr_import: async + async/yield -> sync. 

I'm noticing that it's saner to find async (functions that take onComplete) and async/yield (functions that let self = yield) that don't actually need to be async and converting them to sync and fix their callers yield func(self.cb) -> func();

This then results in the callers having fewer and fewer yields and eventually none and become a candidate to async -> sync.

Otherwise if you go the other way, you end up trying to async -> sync a function but then you need to fix up all of the yields inside that function that needs to be converted to sync and run into even more functions..

Comment 6

[Ed Lee :Mardak]

Buhwahha. A bunch more stuff have been switched to sync like PubKeyManager, CryptoMeta, and now CryptoWrapper.

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/056e4f711ddc#l2.6
CryptoWrapper_decrypt: async + async/yield -> sync. Fix up bookmark/client decrypt. 

The bookmark decrypt loses it's crazy inject between callback magic..

-decrypt: function PlacesItem_decrypt(onComplete, passphrase) {
-  CryptoWrapper.prototype.decrypt.call(this, Utils.bind2(this, function(ret) {
-    // Convert the abstract places item to the actual object type
-    if (!this.deleted)
-      this.__proto__ = this.getTypeObject(this.type).prototype;
-    // Give the original callback the result
-    onComplete(ret);
-  }), passphrase);

But it keeps the dynamic __proto__ type switching :D (but now should be easier to read)..

+decrypt: function PlacesItem_decrypt(passphrase) {
+  // Do the normal CryptoWrapper decrypt, but change types before returning
+  let clear = CryptoWrapper.prototype.decrypt.apply(this, arguments);
+  // Convert the abstract places item to the actual object type
+  if (!this.deleted)
+    this.__proto__ = this.getTypeObject(this.type).prototype;
+  return clear;

Comment 7

[Anant Narayanan [:anant]]

Attachment: Switch _applyIncoming to Sync

Getting my feet wet with the new Sync module: Hope this is correct!

Comment 8

[Ed Lee :Mardak]

Comment on attachment 381780 [details] [diff] [review]
Switch _applyIncoming to Sync

>   _applyIncoming: function SyncEngine__applyIncoming(item) {
..
>-      yield this._store.applyIncoming(self.cb, item);
>+      Sync(this._store.applyIncoming)(item);
That's the right way to use Sync if you were to only convert that one line.

>-  applyIncoming: function Store_applyIncoming(onComplete, record) {
>+  applyIncoming: function Store_applyIncoming(rec) {
But if you have this change, it's already a plain synchronous function (there's no async onComplete).

It turns out most things don't actually need the Sync() module. Right now there's only one use of Sync and it's Sync.withCb(asyncOpen), but in fact that's not even needed because we can just do chan.open synchronously.

We only have async everywhere because one thing being async caused everything above it to be async. So once the network request is made sync., all the callers can be made into normal synchronous functions.

Comment 9

[Ed Lee :Mardak]

(In reply to comment #8)
> >+      Sync(this._store.applyIncoming)(item);
> That's the right way to use Sync if you were to only convert that one line.
Actually, I lied. ;) Store_applyIncoming is expecting the correct "this".

Comment 10

[Ed Lee :Mardak]

Attachment: sample stack snip

Just an example of what we're cleaning up:

Calling findCluster when clicking the login button without async, the call stack should eventually be..

WeaveSvc_findCluster()
WeaveSvc__verifyLogin()
NotifyWrap()
WeaveCatchAllWrapper()
WeaveSvc_verifyLogin()
WeaveSvc__login()
NotifyWrap()
WeaveLocalLockWrapper()
WeaveCatchAllWrapper()
WeaveSvc_login()
Login_doOK()

Basically, right now we have 2 or 3 additional async run wrappers between most of those lines.

Comment 11

[Ed Lee :Mardak]

Attachment: super diff so far (up to engines.js de-asyncify)

All the engine stuff has been switched to sync starting with implementing a new Utils.notify instead of Wrap.notify only doing async/yield functions.

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/e241e775325d
Temporarily convert _notify to _notifyAsync and add a Utils.notify. 

Some changesets later and we have a clean engines.js!

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/dd99c5d93f01
Remove Async.sugar from engines.js. 

Attached is a super diff of weave-Sync vs weave(-central?).

Up next is service.js...

Comment 12

[Dan Mills [:thunder]]

Looking great so far!

Comment 13

[Ed Lee :Mardak]

I've switched a bunch of service.js stuff, but I haven't touched anything with localLock yet. Dan, should those be wrapped in a certain order? Some places do catch(notify(lock(func))) while others do catch(lock(notify(func))) ?

There's some other code in service.js that I haven't touched because they don't get called for normal syncing.. like setCluster, verifyPassphrase, createAccount.

But one thing I did change that doesn't get called is Identity_getPassword:
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/ec9374825489
Identity_getPassword: async + async/yield -> sync. Make onGetPassword take one cb to pass back the password.

It seems like it used to be set somewhere to be a async/yield and given the Identity object to set the password. I've changed it to be a function that takes a callback to pass back a password... So now there's 2 places in the code that uses Sync.

Comment 14

[Ed Lee :Mardak]

I just started switching things to Utils.lock instead of Wrap.localLock..
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/671e60b6bdfb

With my latest push:
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/b426d0778bc8

Some reason the status progress bar thing doesn't show up. It just shows Completed. after you see the status bar spin and stop.

I'll investigate on my flight.. boarding time!

Comment 15

[Ed Lee :Mardak]

(In reply to comment #14)
> Some reason the status progress bar thing doesn't show up.
Because everything was synchronous, the chrome caller doesn't actually continue until sync finishes. I fixed this by making the call to Weave.sync() async by wrapping it in a setTimeout.

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/aa36a1bf329c

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-Sync/rev/7fc664527636
Remove async.js and wrap.js and remove remaining references to Async.sugar, etc.

Comment 16

[Ed Lee :Mardak]

Attachment: super diff with async removed

b/chrome/content/fennec-weave-overlay.js |   15 -
 b/chrome/content/load-weave.js           |    2 
 b/chrome/content/login.js                |    3 
 b/chrome/content/pick-sync.js            |    3 
 b/chrome/content/preferences.js          |    6 
 b/chrome/content/status.js               |    4 
 b/chrome/content/wizard.js               |   51 +---
 b/modules/auth.js                        |    3 
 b/modules/base_records/collection.js     |   25 --
 b/modules/base_records/crypto.js         |   95 ++-----
 b/modules/base_records/keys.js           |   24 -
 b/modules/base_records/wbo.js            |   73 ++---
 b/modules/engines.js                     |  104 ++------
 b/modules/engines/bookmarks.js           |    3 
 b/modules/engines/clientData.js          |    4 
 b/modules/engines/cookies.js             |    4 
 b/modules/engines/forms.js               |    9 
 b/modules/engines/history.js             |   20 -
 b/modules/engines/input.js               |    4 
 b/modules/engines/passwords.js           |    3 
 b/modules/engines/prefs.js               |    3 
 b/modules/engines/tabs.js                |    4 
 b/modules/ext/Sync.js                    |  193 +++++++++++++++
 b/modules/identity.js                    |   27 --
 b/modules/resource.js                    |   63 ++---
 b/modules/service.js                     |  383 +++++++++++--------------------
 b/modules/stores.js                      |   21 -
 b/modules/trackers.js                    |    3 
 b/modules/type_records/bookmark.js       |   19 -
 b/modules/type_records/clientData.js     |   13 -
 b/modules/type_records/forms.js          |    3 
 b/modules/type_records/history.js        |    3 
 b/modules/type_records/passwords.js      |    3 
 b/modules/type_records/prefs.js          |    3 
 b/modules/type_records/tabs.js           |    3 
 b/modules/util.js                        |   81 +++++-
 modules/async.js                         |  350 ----------------------------
 modules/wrap.js                          |  165 -------------
 38 files changed, 606 insertions(+), 1189 deletions(-)

Comment 17

[Ed Lee :Mardak]

Pushed some more changes to fix up tests and now they all pass except test_passwords which didn't pass on weave-core anyway. I removed 3 of the test_async tests.

So probably pretty close to landing weave-sync on weave-core now. I'll do some more sanity checks of the wizard.

Comment 18

[Dan Mills [:thunder]]

(In reply to comment #17)
> Pushed some more changes to fix up tests and now they all pass except
> test_passwords which didn't pass on weave-core anyway. I removed 3 of the
> test_async tests.

With my commits from last week, all tests except for test_passwords are *already* passing for me (including the async tests).  So color me confused.

> So probably pretty close to landing weave-sync on weave-core now. I'll do some
> more sanity checks of the wizard.

Awesome!

Comment 19

[Mike Connor [:mconnor]]

We should get this into 0.5

Comment 20

[Ed Lee :Mardak]

~/weave [---]$ hg pull ../weave-Sync/
added 70 changesets with 143 changes to 70 files

Some reason pushlog decided to collapse it all, but click on the expand to see the changes:
http://hg.mozilla.org/labs/weave/pushloghtml?fromchange=960a8df37031&tochange=05a0117393b9