867614 - SimplePush: register() timeout should also be considered at connection failure.

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Bug 863732 implements ping based keep-alive which will eventually discover a socket is down. But if a register() call fails in the interval between pings (tracked by requestTimeoutTimer), that should be considered as a failure to receive a ping from the server, even though the ping timer hasn't fired yet.

Ideally PushService should immediately try to reconnect.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Don't nominate for leo until more critical fixes land. This is not a deal-breaker.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: SimplePush: Failure to register should lead to socket reconnection.

I think the chances of a registration failing due to the server DB not being available or something are more unlikely than just having lost the connection. More importantly, the server process will still respond in that case with an error code. It might take a long time to reply if it is under real load, but in that case too it makes sense to reconnect. In such a case, once the redirection stuff is figured out, the client will be redirected to a more responsive server when it reconnects.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 805742 [details] [diff] [review]
SimplePush: Failure to register should lead to socket reconnection.

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

::: dom/push/src/PushService.jsm
@@ +354,5 @@
> +          // The most likely reason for a registration request timing out is
> +          // that the socket has disconnected. Best to reconnect.
> +          if (requestTimedOut) {
> +            this._shutdownWS();
> +            this._reconnectAfterBackoff();

So, say we have two requests -- one old (defined as |duration > this._requestTimeout|) and one new.

This first time through this loop, we will set |requestTimeOut|, send a TimeoutError, and then delete the pendingRequest.  The next time through, the request is too new.

After this block, we close the socket.

Should we instead call TimeoutError (or something), on all pendingRequests?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: SimplePush: Failure to register should lead to socket reconnection.

That is a good idea, although it would eventually happen anyway, this way we have more predictable state interactions.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b5653c16b1

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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