1482972 - convert uses of "defer" to "new Promise" in shared/transport/

Status: RESOLVED FIXED

(DevTools :: General, defect, P4)

Comment 1

[Jennifer Wilde]

Attachment: hg changeset patch

Proposed patch for cleanup of devtools/shared/transport
Converts defer() to new Promise

Comment 2

[Soledad Penades [:sole] [:spenades]]

Thank you, Jennifer! We need to review the code before merging the patch. I'll look for a reviewer.

Comment 3

[Soledad Penades [:sole] [:spenades]]

Comment on attachment 8999724 [details] [diff] [review]
hg changeset patch

Alex, can you have a look? :) Thanks

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Hello Jennifer, thanks a lot for your patch.
I pushed it to TRY [1], our CI infrastructure which will run all the test we have to make sure everything is still working as expected.
This part of the code is very tricky and because of the differences of timing between defer and DOM Promise we might have some surprises.
Let's see how it goes !


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4f71b1fb4b3fe1b0078e1d36d109ed95e4975b6

Comment 5

[Jennifer Wilde]

Thank y'all so much for helping me figure out how to kick off the process for this patch!
This is my first time contributing to an open source project so it's extremely appreciated.

It looks like there are six surprises from the try server. What's the process for running the relevant parts of or the whole test suite locally on my machine so that I can attempt a fix and not have to put any load on the try server?

Also, if you have any recommendations for where to start that would be appreciated as well.
All that comes to mind right now is maybe using the Promise.jsm module instead of DOM Promises.

Thank you again, Soledad and Nicolas!

Comment 6

[Soledad Penades [:sole] [:spenades]]

Haha, I like how you call them 'surprises' :)

The test failures are in the Chrome test suites--you can see that if you hover the surprises: "OS X 10.10 opt Mochitests with e10s test-macosx64/opt-mochitest-devtools-chrome-e10s-3 M-e10s(dt3)"

You can either run this suite locally with ./mach mochitest -f chrome --tag devtools (it takes a long time) or if you click on the surprise, in the lower side of the screen it will show you which test broke and whatever error message. For example here I clicked on the dt3* : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4f71b1fb4b3fe1b0078e1d36d109ed95e4975b6&selectedJob=193883782

It says: "TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_content-type.js"

So that's the test that didn't finish properly. You could run it locally with 

./mach test devtools/client/netmonitor/test/browser_net_content-type.js


We have some docs on running tests here: https://docs.firefox-dev.tools/tests/

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8999724 [details] [diff] [review]
hg changeset patch

Review of attachment 8999724 [details] [diff] [review]:
-----------------------------------------------------------------

Hello Jennifer,

Sorry for the review delay.
This is great work, and TRY looks fine to me !
I only have a few comments and questions, but that's already very nice in such a hairy part of the code :)

Also I'm not sure the commit message is properly formatted. It should be:

```
Bug 1482972 - convert uses of "defer" to "new Promise" in shared/transport/; r=nchevobbe

<summary>
```

Here we don't really need the <summary> part since the title of the commit is already self explanatory.

One last thing, we're in the process of deprecating submitting patch via the "attach file" mechanism to favor a better solution: Phabricator. You can read the doc to know how to setting it up https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html .
If that's too much for now, feel free to attach your next patch as a file too, that's fine for me :)

Thanks again !

::: devtools/shared/transport/packets.js
@@ +229,5 @@
>   */
>  function BulkPacket(transport) {
>    Packet.call(this, transport);
>    this._done = false;
> +  this._resolve;

nit: Could we call this `_readyForWritingResolve` instead so it's really obvious what this "resolve" is ?

Or maybe, we could not bind `_resolve` to `this`, and add it as a property of _readyForWriting. like: 

```
let _resolve;
this._readyForWriting = new Promise((resolve) => {
  _resolve = resolve;
});
this._readyForWriting.resolve = _resolve;
```

this way, we can keep the "defer feel" and keep calling this._readyForWriting.resolve().
What do you think of this ?

::: devtools/shared/transport/stream-utils.js
@@ +73,5 @@
>      this.output.init(output, BUFFER_SIZE);
>    }
>    this._length = length;
>    this._amountLeft = length;
> +  this._resolve;

How would you feel to put `resolve` and `reject` on `this._deferred` instead of binding them to `this` ? (See previous comment)

::: devtools/shared/transport/tests/unit/test_queue.js
@@ -162,3 @@
>    });
> -
> -  return compareDeferred.promise.then(cleanup_files);

Don't we still need to call cleanup_files ?

Comment 8

[Jennifer Wilde]

Attachment: Bug 1482972 - convert uses of "defer" to "new Promise" in shared/transport/; r=nchevobbe

Comment 9

[Jennifer Wilde]

Hey Nicolas,

I've uploaded a new patch to Phabricator, thank you for your review!

Please let me know if there's anything else that seems off in its formatting or construction.

I decided to go with adding the resolves to the properties of _readyForWriting and _deferred in devtools/shared/transport/packets.js and devtools/shared/transport/stream-utils.js respectively.

Also yes, we do still need to call cleanup_files, that was an error of omission on my part, thank you for catching it!

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9003789 [details]
Bug 1482972 - convert uses of "defer" to "new Promise" in shared/transport/; r=nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.

Comment 11

[Soledad Penades [:sole] [:spenades]]

Thank you, Jennifer! Super good for a first contribution.

Nicolas, will you land this when try is green? Merci :)

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

yes I will.
TRY is almost over, and so far so good :)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b34fe8dbf3f
convert uses of "defer" to "new Promise" in shared/transport/; r=nchevobbe

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/9b34fe8dbf3f

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Hello Jennifer, your patch is now on Firefox Nightly 🎉
Thanks a lot !