1206163 - Retry failed register requests

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

Currently, if the connection to the Push service fails, we reject all pending requests with an `AbortError`. Instead, we should keep those requests around and retry after reconnecting.

Comment 1

[Lina Butler [:lina]]

That should be "pending register requests." The WebSocket client code don't track other requests.

Comment 2

[Lina Butler [:lina]]

Attachment: 0001-Bug-1206163-Send-unacknowledged-register-requests-on.patch

Dragana, could you have a look at this, please? This changes the queuing logic for outgoing WebSocket requests.

Comment 3

[Dragana Damjanovic [:dragana]]

Comment on attachment 8664554 [details] [diff] [review]
0001-Bug-1206163-Send-unacknowledged-register-requests-on.patch

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

This need a bit rewriting. Maybe also add a test for the case that will not work with this patch. I think we do not have such a test. "The socket is connected and a registration is requested"

::: dom/push/PushServiceWebSocket.jsm
@@ +990,5 @@
>                                                   resolve: resolve,
>                                                   reject: reject,
>                                                   ctime: Date.now()
>                                                  };
>          this._queueRequest(data);

instead of: if (data.messageType != "register") 
in _queueRequest, you could just delete this line here. 

But anyway this needs to be rewritten, see below.

@@ +1046,2 @@
>    _queueRequest(data) {
> +    if (data.messageType != "register") {

this is not going to work. It is only working if socket is not connected when request arrives, i.e. if finishHandShake is called.


It probably should be:
if (data.messageType != "register" || this._currentState == STATE_READY) {

maybe nicer:
if (this._currentState != STATE_READY && !this._notifyRequestQueue) {
  let promise = new Promise((resolve, reject) => {
    this._notifyRequestQueue = resolve;
  });
  this._enqueue(_ => promise);
}

if (data.messageType != "register" || this._currentState == STATE_READY) {
  this._enqueue(_ => this._send(data));
}

But still with this it can happen that we send request twice :)

Comment 4

[Lina Butler [:lina]]

(In reply to Dragana Damjanovic [:dragana] from comment #3)
> Comment on attachment 8664554 [details] [diff] [review]
> 0001-Bug-1206163-Send-unacknowledged-register-requests-on.patch
> 
> Review of attachment 8664554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This need a bit rewriting. Maybe also add a test for the case that will not
> work with this patch. I think we do not have such a test. "The socket is
> connected and a registration is requested"

Oops, thanks for catching that! Ben and I extended `test_reconnect_retry.js` to send a second register after reconnect. The latest patch also calls `this._send(data)` directly, though I can change that to `this._enqueue(_ => this._send(data))` if you'd like.

Comment 5

[Lina Butler [:lina]]

Attachment: 0001-Bug-1206163-Send-unacknowledged-register-requests-on.patch

Comment 6

[Dragana Damjanovic [:dragana]]

Comment on attachment 8667100 [details] [diff] [review]
0001-Bug-1206163-Send-unacknowledged-register-requests-on.patch

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

With this implementation it can happen that some register request are sent 2 time, we are ok with this extra work on the server. The channelId will be the same for 2 requests. 

The solution could be to enqueue request and only to add them to the requestQueue just before they are sent, but this is extra work so if we are ok from server side to get some, probably very rare, duplicate request we do not need this change.

Comment 7

[Lina Butler [:lina]]

(In reply to Dragana Damjanovic [:dragana] from comment #6)
> With this implementation it can happen that some register request are sent 2
> time, we are ok with this extra work on the server. The channelId will be
> the same for 2 requests.

Not sure I follow. Is this the case where the server handles the register request, but the connection is dropped before it can reply?

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Kit Cambridge [:kitcambridge] (PTO from 2015-10-06 to 2015-10-16) from comment #7)
> (In reply to Dragana Damjanovic [:dragana] from comment #6)
> > With this implementation it can happen that some register request are sent 2
> > time, we are ok with this extra work on the server. The channelId will be
> > the same for 2 requests.
> 
> Not sure I follow. Is this the case where the server handles the register
> request, but the connection is dropped before it can reply?

STATE_READY is set before the requests in the requestQueue are queued for re-registering. For the incoming register requests between setting STATE_READY and putting the re-requestions into a queue, the register requests will be sent 2 times.

It is possible to solve this by generating and setting channelid just before send, so that it is possible to distinguish the requests that needs to be resent and the new coming requests.

So this is not big of a deal, you can decide if you want to fix it.

Comment 9

[Lina Butler [:lina]]

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

Comment 10

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32c3f77b40671d80e6fcbcedf7f6b69c6e5c7121
Bug 1206163 - Retry failed register requests on reconnect. r=dragana

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/32c3f77b4067