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

RESOLVED FIXED in Firefox 63

Status

defect
P4
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: jennowilde, Assigned: jennowilde)

Tracking

(Blocks 1 bug)

unspecified
Firefox 63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

11 months ago
No description provided.
Assignee

Updated

11 months ago
Blocks: 1283869
Assignee

Comment 1

11 months ago
Posted patch hg changeset patch (obsolete) — Splinter Review
Proposed patch for cleanup of devtools/shared/transport
Converts defer() to new Promise
Thank you, Jennifer! We need to review the code before merging the patch. I'll look for a reviewer.
Priority: -- → P4
Comment on attachment 8999724 [details] [diff] [review]
hg changeset patch

Alex, can you have a look? :) Thanks
Attachment #8999724 - Flags: review?(poirot.alex)
Assignee: nobody → jennowilde
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
Assignee

Comment 5

11 months ago
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!
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/
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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 ?
Attachment #8999724 - Flags: review?(poirot.alex) → review?(nchevobbe)
Assignee

Comment 9

10 months ago
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 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.
Attachment #9003789 - Flags: review+
Thank you, Jennifer! Super good for a first contribution.

Nicolas, will you land this when try is green? Merci :)
Flags: needinfo?(nchevobbe)
yes I will.
TRY is almost over, and so far so good :)
Flags: needinfo?(nchevobbe)

Comment 13

10 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b34fe8dbf3f
convert uses of "defer" to "new Promise" in shared/transport/; r=nchevobbe

Comment 14

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b34fe8dbf3f
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Attachment #8999724 - Attachment is obsolete: true
Attachment #8999724 - Flags: review?(nchevobbe)
Hello Jennifer, your patch is now on Firefox Nightly 🎉
Thanks a lot !
You need to log in before you can comment on or make changes to this bug.