1355677 - Make all Sync network requests promise/async based

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

Now that bug 1343158 has some traction I thought I'd try and make a start here. We should wait for both that bug and 56 to land this, but thought I'd get it up now.

My previous attempts at this work kinda worked from the outside-in (eg, make processIncoming promise based and work down) which ended up needing to change the entire world. I think a better approach is inside-out (eg, making the lowest level of the call stacks promise based, hoisting promiseSpinningly calls up to the functions that remain synchronous). This has the downside of sprinkling more promiseSpinningly calls, but allows a more iterative approach.

This patch unifies AsyncResource and Resource making them exclusively promise-based. A few other simple methods also were made promise based while I was there. The patch is large, but hopefully it is fairly easy to review as most changes are somewhat mechanical.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1355677 - Make all Sync network requests promise based.

Review commit: https://reviewboard.mozilla.org/r/129262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129262/

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8857298 [details]
Bug 1355677 - Make all Sync network requests promise based.

https://reviewboard.mozilla.org/r/129262/#review131756

::: services/sync/tests/unit/test_resource_async.js
(Diff revision 1)
>    do_check_eq(uri1.query, uri2.query);
>  
>    run_next_test();
>  });
>  
> -add_test(function test_not_sending_cookie() {

Note that this test is broken on central - |this.response| is undefined, but because it is run in the callback it doesn't make the test fail :( I can't see how it could be made to work without significant effort (eg, there's no code anywhere setup to return "COOKIE!" as a response), so just nuked it.

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8857298 [details]
Bug 1355677 - Make all Sync network requests promise based.

https://reviewboard.mozilla.org/r/129262/#review132032

This looks excellent, thanks for getting the ball rolling! I agree that doing this conversion gradually with the help of `Async.promiseSpinningly` is a good way to go. I left some nits, all of which you're welcome to ignore. :-)

::: services/sync/modules/record.js:63
(Diff revision 1)
> -  upload: function upload(resource) {
> +  async upload(resource) {
>      if (!(resource instanceof Resource)) {
>        throw new Error("First argument must be a Resource instance.");
>      }
>  
> -    return resource.put(this);
> +    return await resource.put(this);

Nit: You can drop the `await` when you return, unless you're using `return await` for style.

::: services/sync/modules/resource.js:181
(Diff revision 1)
>  
> -  _onProgress: function Res__onProgress(channel) {},
> +  _onProgress(channel) {},
>  
> -  _doRequest: function _doRequest(action, data, callback) {
> +  _doRequest(action, data) {
>      this._log.trace("In _doRequest.");
> -    this._callback = callback;
> +    let result = new Promise((resolve, reject) => {

A small suggestion: move the entire body of this function into `new Promise((resolve, reject) => { /* ... */ })`, and remove the `try...catch`. That will give us error handling for free by transforming thrown exceptions into rejections.

::: services/sync/modules/resource.js:182
(Diff revision 1)
> -  _onProgress: function Res__onProgress(channel) {},
> +  _onProgress(channel) {},
>  
> -  _doRequest: function _doRequest(action, data, callback) {
> +  _doRequest(action, data) {
>      this._log.trace("In _doRequest.");
> -    this._callback = callback;
> +    let result = new Promise((resolve, reject) => {
> +      this._resolveReject = { resolve, reject };

Nit: Maybe `resolver` or `deferred` is a better name.

::: services/sync/modules/resource.js:233
(Diff revision 1)
> +      let error;
> +      if (Async.isShutdownException(ex)) {
> +        error = ex;
> +      } else {
> +        this._log.warn("${action} request to ${url} failed: ${ex}",
> +                       { action: this.method, url: this.uri.spec, ex});

Can you use `channel.requestMethod` here and drop `this.method`?

::: services/sync/modules/resource.js:243
(Diff revision 1)
> +        let chanStack = [];
> +        if (ex.stack)
> +          chanStack = ex.stack.trim().split(/\n/).slice(1);
> +        let requestStack = error.stack.split(/\n/).slice(1);
> +
> +        // Strip out the args for the last 2 frames because they're usually HUGE!

I'm not sure I understand this fancy stack rewriting. Do we still need it in the new promise-based world?

::: services/sync/tests/unit/test_resource_async.js:543
(Diff revision 1)
> -add_test(function test_preserve_exceptions() {
> +add_task(async function test_preserve_exceptions() {
>    _("Error handling in ChannelListener etc. preserves exception information");
>    let res11 = new AsyncResource("http://localhost:12345/does/not/exist");
> -  res11.get(function(error, content) {
> +  try {
> +    await res11.get();
> +    throw new Error("expected get to fail");

Now that `get` returns a promise, what do you think of using `Assert.rejects` here and elsewhere?

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8857298 [details]
Bug 1355677 - Make all Sync network requests promise based.

https://reviewboard.mozilla.org/r/129262/#review132090

This is great. Some comments:

- Eventually we'll need to change our strategy for handling shutdown exeptions, since we won't throw any if we aren't spinning event loops. I think the check for them inside catch clauses is probably harmless, even if we won't throw them, but it seems likely that we want to give up on syncing ASAP when shutdown starts, since it's likely to fail anyway.

- We'll probably need to update aboutsync to do `Async.promiseSpinningly(Promise.resolve(somePossiblyAsyncFunction()))` in some places (just a quick look, inside `LocalProvider.prototype.promiseCollection` might be all, but I could be wrong).

- I'd also kind of like to file bugs to move some of the spinning further up (down?) the call stack, unless we want to rest it all on bug 1210296 -- which seems like a lot of work, and "removing spinning event loops piecemeal" sounds like the kind of work that we might be able to use to familiarize someone (e.g. :Grisha) with the codebase, since it requires touching multiple parts, while still being fairly mechanical and easy. (I guess the stack trace issue in that bug is likely still a problem also...)  
  
  But it's up to you. It's probably not a great idea since I don't know how we could really break it down into smaller bugs (obviously one bug per promiseSpinningly would be both horrible and insane...), but I figured I'd mention it.

::: services/sync/modules/record.js:823
(Diff revision 1)
>        this.batch = batch;
>        this.commit = commit;
>        for (let [header, value] of headers) {
>          this.setHeader(header, value);
>        }
> -      return Resource.prototype.post.call(this, data);
> +      return Async.promiseSpinningly(Resource.prototype.post.call(this, data));

Can we make this return the promise, and have calling the poster do the spin?

This seems like low hanging fruit, but I'm okay either way (obviously there's a point at which it becomes too much work to move the calls up the stack to be worth handling in this patch, and you might have a better idea where that point is than me).

::: services/sync/modules/resource.js:182
(Diff revision 1)
> -  _onProgress: function Res__onProgress(channel) {},
> +  _onProgress(channel) {},
>  
> -  _doRequest: function _doRequest(action, data, callback) {
> +  _doRequest(action, data) {
>      this._log.trace("In _doRequest.");
> -    this._callback = callback;
> +    let result = new Promise((resolve, reject) => {
> +      this._resolveReject = { resolve, reject };

Agreed on naming it deferred, as it's almost the same as the Promise.jsm's Promise.defer (although that function's result also has the promise as a property). Would it be worth moving something similar into utils (or commonutils?)... Assuming we're avoiding Promise.jsm forever, I mean.

::: services/sync/modules/resource.js:243
(Diff revision 1)
> +        let chanStack = [];
> +        if (ex.stack)
> +          chanStack = ex.stack.trim().split(/\n/).slice(1);
> +        let requestStack = error.stack.split(/\n/).slice(1);
> +
> +        // Strip out the args for the last 2 frames because they're usually HUGE!

Agreed 100% (and I'd like to point out that keeping it could make debugging much harder if we change too much -- e.g. if those first two frames have useful data).

Assuming it is needed, why is the loop repeated? This was mostly moved from elsewhere (the old line 404), where it doesn't *seem* to be repeated... or is that just me misreading mozreview?

Comment 5

[Lina Butler [:lina]]

As an aside, it looks like we can use `fetch` from chrome code (ref. https://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm#18; added in bug 1155898). \o/

However, I don't think Sync can use it yet, because our `fetch` implementation doesn't support streaming response bodies (bug 1128959). (╯°□°）╯︵ ┻━┻

Comment 6

[Richard Newman [:rnewman]]

(In reply to Kit Cambridge [:kitcambridge] (He/him; UTC-8) from comment #5)

> However, I don't think Sync can use it yet, because our `fetch`
> implementation doesn't support streaming response bodies (bug 1128959).
> (╯°□°）╯︵ ┻━┻

Be really careful of changing stacks without a clear win! You'll be playing whack-a-mole with headers and encodings and automatic translation into numbers and which error codes count as errors and and and.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8857298 [details]
Bug 1355677 - Make all Sync network requests promise based.

https://reviewboard.mozilla.org/r/129262/#review133856

I don't think this is a bad idea. I have no way of knowing if you missed any spots, of course!

More broadly: if you start down this road, you're committing to continuing down it until you've killed most of those `promiseSpinningly` calls. Be aware that this is a temporary increase in complexity, and it's only worthwhile once we reach the blessed valley on the other side of the mountain.

::: services/sync/modules/engines.js:1854
(Diff revision 1)
>        throw response;
>      }
>      this._resetClient();
>    },
>  
> -  removeClientData() {
> +  async removeClientData() {

I remember one of the concerns we had about an async switchover is that the signature for `Engine` is implemented by third-party sync engines. Are you sure that this change isn't going to break Stylish Sync or ABP?

Think about a way to allow those codebases to do the right thing.

::: services/sync/tests/unit/head_errorhandler_common.js:115
(Diff revision 1)
>  
> -  generateAndUploadKeys() {
> +  async generateAndUploadKeys() {
>      generateNewKeys(Service.collectionKeys);
>      let serverKeys = Service.collectionKeys.asWBO("crypto", "keys");
>      serverKeys.encrypt(Service.identity.syncKeyBundle);
> -    return serverKeys.upload(Service.resource(Service.cryptoKeysURL)).success;
> +    return (await serverKeys.upload(Service.resource(Service.cryptoKeysURL))).success;

Can we break this into two lines? Having an `await` and a `return` and a field accessor in the same expression confuses me.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8857298 [details]
Bug 1355677 - Make all Sync network requests promise based.

*shakes fist at MozReview*

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Thanks for the thorough reviews!

(In reply to Kit Cambridge [:kitcambridge] (He/him; UTC-8) from comment #3)
> Nit: You can drop the `await` when you return, unless you're using `return
> await` for style.

Yeah, I was. I asked on #developers for style opinions, and the 3 people who responded agreed we shouldn't await on the return. That in turn means one of the functions doesn't need the "async" (as it has no await's), so I removed that too.

> A small suggestion: move the entire body of this function into `new
> Promise((resolve, reject) => { /* ... */ })`, and remove the `try...catch`.
> That will give us error handling for free by transforming thrown exceptions
> into rejections.

Yeah, I did that initially to keep the patch small, but I agree, so I did it.

> Nit: Maybe `resolver` or `deferred` is a better name.

Yep, _deferred it is.

> Can you use `channel.requestMethod` here and drop `this.method`?

Sadly not - a timeout error calls this without a channel.

> I'm not sure I understand this fancy stack rewriting. Do we still need it in
> the new promise-based world?

Good question - as Thom says, it's very fragile, so I wonder if we need it even before this change? The comments don't mention what those 2 mythical massive frames are supposed to be. The comments also lead me to believe "ex" was going to be an async stack, but it's always created directly up the stack, so that's not it either. So yeah, dead!

> Now that `get` returns a promise, what do you think of using
> `Assert.rejects` here and elsewhere?

Assert.rejects doesn't return the error :( But it should, so I opened bug 1357625. But then that seems a little tricker is some stupid edge case, so I'm not sure it will land :) So I've got another patch that relies on that, which I'll fold in if it lands and discard otherwise.

(In reply to Thom Chiovoloni [:tcsc] from comment #4)
> - Eventually we'll need to change our strategy for handling shutdown
> exeptions, since we won't throw any if we aren't spinning event loops. I

Yeah, that's a great point. I don't think this patch makes that worse (as we should spin just as often, but just from a different place), but that's something we should consider soon.

IME, even once everything is promise-based, we will still need to explicitly yield to the event loop to prevent jank. So I wonder if we will just need a promise-based function like Utils.nextTick() that we ensure is called in loops and we can have this check the shutdown flag. Better ideas welcome :)

> - We'll probably need to update aboutsync to do
> `Async.promiseSpinningly(Promise.resolve(somePossiblyAsyncFunction()))` in
> some places (just a quick look, inside
> `LocalProvider.prototype.promiseCollection` might be all, but I could be
> wrong).

Yeah, I pushed a change for that, but haven't resubmitted it with recent changes yet.

> - I'd also kind of like to file bugs to move some of the spinning further up
> (down?) the call stack, unless we want to rest it all on bug 1210296 --
> which seems like a lot of work,

I agree with the sentiment, but I'm not sure it makes sense to file other bugs until someone decides what that next patch might look like :)

> and "removing spinning event loops
> piecemeal" sounds like the kind of work that we might be able to use to
> familiarize someone (e.g. :Grisha) with the codebase, since it requires
> touching multiple parts, while still being fairly mechanical and easy. (I
> guess the stack trace issue in that bug is likely still a problem also...)

I think that's a great idea, but I don't want to decide up-front what that would look like. IOW, I think we should encourage that, but let the bug be opened by whoever decides to have a bite :)

Richard's comment on breaking other engines is likely to be a constraint, at least until old-school addons don't work on release (57?)

> Can we make this return the promise, and have calling the poster do the spin?

Yeah, that still moves the spin into the queue itself, so I did that.

> Agreed 100% (and I'd like to point out that keeping it could make debugging
> much harder if we change too much -- e.g. if those first two frames have
> useful data).

Yeah, see above, I agree and killed it :)

> Assuming it is needed, why is the loop repeated? This was mostly moved from
> elsewhere (the old line 404), where it doesn't *seem* to be repeated... or
> is that just me misreading mozreview?

That was a mistake, thanks :)

(In reply to Richard Newman [:rnewman] from comment #7)
> More broadly: if you start down this road, you're committing to continuing
> down it until you've killed most of those `promiseSpinningly` calls. Be
> aware that this is a temporary increase in complexity, and it's only
> worthwhile once we reach the blessed valley on the other side of the
> mountain.

Agreed 100%, but we are committed to that valley :)

> I remember one of the concerns we had about an async switchover is that the
> signature for `Engine` is implemented by third-party sync engines. Are you
> sure that this change isn't going to break Stylish Sync or ABP?

Yeah, that's a good point. I changed the patch so a 3rd-party engine without a promise-based version of this function works, and in this specific case, an engine calling our base implementation is harmless as it is a no-op.

But that's a great point, but given this work isn't a seriously high priority, I suspect xul addons will be dead by the time we worry about that.

> Can we break this into two lines? Having an `await` and a `return` and a
> field accessor in the same expression confuses me.

I'll upload a new version and I wouldn't mind another quick look from anyone with the patience :)

Thanks!

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8857298 [details]
Bug 1355677 - Make all Sync network requests promise based.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129262/diff/1-2/

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8857298 [details]
Bug 1355677 - Make all Sync network requests promise based.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129262/diff/2-3/

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8857298 [details]
Bug 1355677 - Make all Sync network requests promise based.

https://reviewboard.mozilla.org/r/129262/#review134938

Still LGTM, modulo questions about the `await`s.

::: services/sync/tests/unit/head_errorhandler_common.js:103
(Diff revision 3)
>      // Make sync fail due to changed credentials. We simply re-encrypt
>      // the keys with a different Sync Key, without changing the local one.
>      let newSyncKeyBundle = new SyncKeyBundle("johndoe", "23456234562345623456234562");
>      let keys = Service.collectionKeys.asWBO();
>      keys.encrypt(newSyncKeyBundle);
>      keys.upload(Service.resource(Service.cryptoKeysURL));

Does this `upload` call need an `await`?

::: services/sync/tests/unit/test_corrupt_keys.js:74
(Diff revision 3)
>  
>      // Bump version on the server.
>      let m = new WBORecord("meta", "global");
>      m.payload = {"syncID": "foooooooooooooooooooooooooo",
>                   "storageVersion": STORAGE_VERSION};
>      m.upload(Service.resource(Service.metaURL));

`await` here, too?

::: services/sync/tests/unit/test_service_detect_upgrade.js:253
(Diff revision 3)
>      _("Bumping version.");
>      // Bump version on the server.
>      let m = new WBORecord("meta", "global");
>      m.payload = {"syncID": "foooooooooooooooooooooooooo",
>                   "storageVersion": STORAGE_VERSION + 1};
>      m.upload(Service.resource(Service.metaURL));

And here?

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8857298 [details]
Bug 1355677 - Make all Sync network requests promise based.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129262/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b858187d6cb8
Make all Sync network requests promise based. r=kitcambridge,rnewman,tcsc

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b858187d6cb8