1216554 - Request sent after client close stay pending

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

In order to better handle unexpected loss of connection,
we should reject/throw/do-something when we loose the connection.
A request should never stall, otherwise it means we have a stalling promise and possibly a broken UI that never cleans up (typically: WebIDE toolboxes that never closes)

Today, request that are send after the transport is closed are not rejected, nothing happens and they stall. Ideally, we would immediately reject them with an error or throw.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

With a test!

Comment 2

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

Comment on attachment 8676269 [details] [diff] [review]
patch v1

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

This seems like the right idea!

However, there can also be queued requests in `_pendingRequests` (and `_activeRequests` too I guess) if the connection was open when `request` was first called, but then it closed before the request was sent.

So, we should probably go through these request queues at close time and reject them.

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> Comment on attachment 8676269 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8676269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like the right idea!
> 
> However, there can also be queued requests in `_pendingRequests` (and
> `_activeRequests` too I guess) if the connection was open when `request` was
> first called, but then it closed before the request was sent.

I've seen that, but I've always seen them empty in my tests.
But yes, that would be great also.
I thought this patch would already break many tests, and was conservatice about this.
But it looks like it isn't:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=eacee2dae9be

So I'll give this a try and see if it stress our tests

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Try is closed now, but here is a new patch that also cleans active/pending requests.
Hopefully it doesn't blow up our tests!
I moved the map cleanup to onClosed so that it is cleaned up even if we don't call client.close.

Comment 5

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

Comment on attachment 8676795 [details] [diff] [review]
patch v2

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

::: devtools/shared/client/main.js
@@ +1051,3 @@
>      this.emit("closed");
>  
> +    // Reject all pending requests

Nit: pending and active

@@ +1055,5 @@
> +      let msg = "'" + request.request.type + "' " + type + " request packet "+
> +                "to '" + request.request.to + "' " +
> +                "can't be sent as the connection just closed.";
> +      let packet = { error: "connectionClosed", message: msg };
> +      request.emit("json-reply", packet);

Shouldn't we also reject the promise too?

Let's save the `deferred` inside the `Request` object, so you can access it here to reject it.

Comment 6

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

Comment on attachment 8676795 [details] [diff] [review]
patch v2

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

Talked on IRC, the message listener already rejects the promise when there's an error.

Only a nit to fix, then.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

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

I had to tweak the code in DebuggerClient.onClosed a little bit
to handle the cases where request objects stored in maps
don't have a request attribute.
It happens when we call expectReply with a callback, for the hello packet from root.

Comment 8

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

Comment on attachment 8677370 [details] [diff] [review]
patch v3

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

Good catch!

Comment 9

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/d91e6b148b9077e2695aee5eee744b901e1a8515
Bug 1216554 - Reject requests immediately when the connection is already closed. r=jryans

Comment 10

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

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