1243452 - Make DebuggerClient.close return a promise

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

Sibling of bug 1239276, but instead of connect, targets close function.
I think it will be the last one to convert.

Comment 1

[AJ Kerrigan [:ajkerrigan]]

Attachment: Make DebuggerClient.close return a promise

Since this was a sibling of Bug 1243452, I've tried to follow the same approach
from that bug.

The primary update is in devtools/shared/client/main.js:

- Return a promise from DebuggerClient.close()
- Document the promise return and mark the callback parameter as deprecated

From there, I did some searches to find and update all calls to the close()
method which passed a callback parameter.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8716737 [details] [diff] [review]
Make DebuggerClient.close return a promise

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

Great, thanks for working on this!  I believe you found all the right cases to clean up.

::: devtools/shared/client/main.js
@@ +368,5 @@
>     * @param aOnClosed function
>     *        If specified, will be called when the debugging connection
> +   *        has been closed. This parameter is deprecated - please use
> +   *        the returned Promise.
> +   *

Nit: We typically don't use a blank line between @param and @return

@@ +395,5 @@
>      // there is no need to detach client
>      // as we won't be able to send any message.
>      if (this._closed) {
>        cleanup();
> +      resolve();

We should `return deferred.promise;` after this, so the rest of the method is skipped as before.

Comment 3

[AJ Kerrigan [:ajkerrigan]]

Attachment: Make DebuggerClient.close return a promise

Good call, thanks for the review. Those changes should be in now.

Comment 4

[AJ Kerrigan [:ajkerrigan]]

Comment on attachment 8716937 [details] [diff] [review]
Make DebuggerClient.close return a promise

Submitted this for re-review unnecessarily.

Comment 5

[AJ Kerrigan [:ajkerrigan]]

Comment on attachment 8716937 [details] [diff] [review]
Make DebuggerClient.close return a promise

Fixed items from :jryans conditional review+

Comment 6

[AJ Kerrigan [:ajkerrigan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6960b4ecbba9

Comment 7

[AJ Kerrigan [:ajkerrigan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9519d1665725

Comment 8

[AJ Kerrigan [:ajkerrigan]]

My changes passed review a while back, but try runs kept showing too many instances of intermittent failures. I suspect the problem(s) are related to promise handling in tests that use DebuggerClient.close, but despite a bunch of test refactoring I wasn't able to get try runs to be green at a level I was comfortable with.

I don't have much time available lately to work on issues like this, and I think that's going to be the way for the foreseeable future. I'm leaving this update and unassigning in case someone else wants to pick it up before I get back to it.

Comment 9

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c60bf0e58c04

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1243452 - Make DebuggerClient.close return a Promise.

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

Comment 11

[Alexandre Poirot [:ochameau]]

A bunch of code already expect it to return a Promise :/
https://dxr.mozilla.org/mozilla-central/search?q=yield+client.close&redirect=false

Also, in my patch, I'm also converting all callsites to use the promise form,
while keeping the callback argument. I did that while thinking about addons, but may be that's useless?

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8786970 [details]
Bug 1243452 - Make DebuggerClient.close return a Promise.

https://reviewboard.mozilla.org/r/75812/#review73784

Looks good, thanks for doing this cleanup!

::: devtools/server/tests/browser/head.js:90
(Diff revision 1)
>  /**
>   * Close a debugger client's connection.
>   * @param {DebuggerClient}
>   * @return {Promise} Resolves when the connection is closed.
>   */
>  function closeDebuggerClient(client) {

Seems like this can be inlined.

::: devtools/shared/client/main.js:353
(Diff revision 1)
>      this._transport.ready();
>      return deferred.promise;
>    },
>  
>    /**
>     * Shut down communication with the debugging server.

Update comment to note it returns a promise.

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 8786970 [details]
Bug 1243452 - Make DebuggerClient.close return a Promise.

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

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> A bunch of code already expect it to return a Promise :/
> https://dxr.mozilla.org/mozilla-central/search?q=yield+client.
> close&redirect=false
> 
> Also, in my patch, I'm also converting all callsites to use the promise form,
> while keeping the callback argument. I did that while thinking about addons,
> but may be that's useless?

Yes, I think you should preserve the callback argument for add-ons.  That seems best to me.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52851295b34f
Make DebuggerClient.close return a Promise. r=jryans

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/52851295b34f