855906 - Set pingInterval on websocket

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

Rather than wait for TCP to timeout on error, which can be upto 30 minutes long, use the WebSocket protocol's ping/pong mechanism to detect connection loss.

ref bug 849364, WebSocketChannel exposes pingInterval and pingTimeout.

Comment 1

[Jason Duell]

We're not using TCP keepalive (which only checks every 2 hours IIRC), so if we're not sending data through the websocket we may not ever get notification of the TCP socket going down.  Is that a problem?  I assume so.

Setting pingInterval is a one-line fix so we may want to do that soon?  If you give me the interval you want to use I can write a patch.

Comment 2

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

55s sounds good.

Comment 3

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

You asked for it :)

Comment 4

[Jason Duell]

Attachment: part1: convert pingInterval to use seconds, not ms

ms seems like a footgun--it's hard to think of a valid use case for sub-second ping intervals.  The prefs.js pref is already using seconds.

Comment 5

[Jason Duell]

Attachment: part2: use ping interval of 55 sec for Update notification websocket

A ping every 55 sec strikes me as power-munchy, but dougt knows the UDP wakeup stuff, so maybe it's fine in the B2G case.  Note that we're apparently planning to use this in Android too--I very much doubt we'll want ping-per-minute there, so do we need a followup to set this differently depending on #if B2G ?

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 733137 [details] [diff] [review]
part2: use ping interval of 55 sec for Update notification websocket

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

nsm - please create a follow up bug to investigate using a variable interval based on if the network drops or not (details can put put in the new bug)

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 733134 [details] [diff] [review]
part1: convert pingInterval to use seconds, not ms

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

::: netwerk/protocol/websocket/BaseWebSocketChannel.cpp
@@ +125,2 @@
>  {
> +  *aSeconds = mPingInterval ? mPingInterval / 1000 : 0;

just mPingInterval / 1000 right? (i.e. you don't need the conditional)

@@ +145,2 @@
>  {
> +  *aSeconds = mPingResponseTimeout ? mPingResponseTimeout / 1000 : 0;

just mPingResponseTimeout / 1000, right?

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 733137 [details] [diff] [review]
part2: use ping interval of 55 sec for Update notification websocket

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

I remain concerned about battery and bandwidth issues here. But lets go ahead and see who is right.

Comment 9

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

Comment on attachment 733137 [details] [diff] [review]
part2: use ping interval of 55 sec for Update notification websocket

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

Just a magic number issue. If you don't mind, I can just fix it up and land it.

I'll file a follow up bug for intelligent ack and some other approaches. Appropriate intervals seems to be something for Tef/Mozilla people with actual hardware on their hands to find out.

::: dom/push/src/PushService.js
@@ +537,5 @@
>  
>      debug("serverURL: " + uri.spec);
>      this._wsListener = new PushWebSocketListener(this);
>      this._ws.protocol = "push-notification";
> +    this._ws.pingInterval = 55;

this._ws.pingInterval = this._prefs.get("websocketPingInterval"); ?

You'll want to add services.push.websocketPingInterval to b2g/app/b2g.js with the other push prefs.

Comment 10

[Jason Duell]

> If you don't mind, I can just fix it up and land it.

all yours!

Comment 11

[Jason Duell]

Attachment: part1, v2: convert pingInterval to use seconds, not ms

This just removes ternary operator per PAtrick's observation in comment 7.  Carrying forward his +r.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5ef06d13fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/9509f6be0095

Comment 13

[Jason Duell]

Nihil: is this going to need to get uplifted?  Will b2g18 deal with detecting dropped websockets w/o this?

Comment 14

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

w/o this, dropped sockets will take a long time to be detected unless the network interface state changes. I'd like it to get uplifted. I usually wait for patches to land on m-c before marking them as tracking b2g18.

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5f5ef06d13fa
https://hg.mozilla.org/mozilla-central/rev/9509f6be0095

Comment 16

[Jason Duell]

requesting tracking per comment 13 and 14

Comment 17

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

@jduell: Thanks for the patches, but some new issues raised at the Madrid work week mean that we'll have to disable this and use a application level ping pong. Turns out nsITimer doesn't trigger when b2g devices are in low power mode (screen off, not used for a few minutes). The solution is to use alarms which is backed by the RTC. Except the alarm API is inaccessible from C++ (current also from JS Gecko code) since there can only be *one* watcher for the RTC timeout event, and that watcher is the Alarms API AlarmService. See bug 862322 for more info.

The patch is still useful on desktop.

Comment 18

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

Is tracking-b2g? still required considering comment 17? I think not.

AFAIK right now push was the only use of pingInterval on b2g and possibly the only use of websocket outside of apps. Apps don't have access to pingInterval anyway.

Comment 19

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

Reflected the fact that SimplePush no longer relies on this feature.

@Chris - Does Android still block on this?

Comment 20

[Chris Peterson [:cpeterson]]

(In reply to Nikhil Marathe from comment #19)
> @Chris - Does Android still block on this?

Nope. Thanks for the heads up.