Closed Bug 867614 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Push Notifications, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Don't nominate for leo until more critical fixes land. This is not a deal-breaker.
Component: General → DOM: Push Notifications
Product: Boot2Gecko → Core
Version: unspecified → Trunk
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.
Attachment #805742 - Flags: review?(doug.turner)
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?
That is a good idea, although it would eventually happen anyway, this way we have more predictable state interactions.
Attachment #806064 - Flags: review?(doug.turner)
Attachment #805742 - Attachment is obsolete: true
Attachment #805742 - Flags: review?(doug.turner)
Attachment #806064 - Flags: review?(doug.turner) → review+
https://hg.mozilla.org/mozilla-central/rev/e7b5653c16b1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.