Set pingInterval on websocket

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
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.
Flags: needinfo?(nsm.nikhil)
Depends on: 849364
55s sounds good.
Flags: needinfo?(nsm.nikhil)
You asked for it :)
Assignee: nsm.nikhil → jduell.mcbugs
Blocks: 834033
Created attachment 733134 [details] [diff] [review]
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.
Attachment #733134 - Flags: review?(mcmanus)
Created attachment 733137 [details] [diff] [review]
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 ?
Attachment #733137 - Flags: review?(doug.turner)
Attachment #733137 - Flags: feedback?(mcmanus)

Comment 6

5 years ago
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)
Attachment #733137 - Flags: review?(doug.turner) → review+
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?
Attachment #733134 - Flags: review?(mcmanus) → review+
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.
Attachment #733137 - Flags: feedback?(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]:
-----------------------------------------------------------------

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.
Attachment #733137 - Flags: review-
> If you don't mind, I can just fix it up and land it.

all yours!
Assignee: jduell.mcbugs → nsm.nikhil
Created attachment 733646 [details] [diff] [review]
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.
Attachment #733646 - Flags: review+
Attachment #733134 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5ef06d13fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/9509f6be0095
Nihil: is this going to need to get uplifted?  Will b2g18 deal with detecting dropped websockets w/o this?
Flags: needinfo?(nsm.nikhil)
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.
Flags: needinfo?(nsm.nikhil)
https://hg.mozilla.org/mozilla-central/rev/5f5ef06d13fa
https://hg.mozilla.org/mozilla-central/rev/9509f6be0095
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
requesting tracking per comment 13 and 14
tracking-b2g18: --- → ?
@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.
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.
tracking-b2g18: ? → ---

Updated

5 years ago
Depends on: 870579
Reflected the fact that SimplePush no longer relies on this feature.

@Chris - Does Android still block on this?
No longer depends on: 822712
Summary: SimplePush: Set pingInterval on websocket → Set pingInterval on websocket
(In reply to Nikhil Marathe from comment #19)
> @Chris - Does Android still block on this?

Nope. Thanks for the heads up.
No longer blocks: 834033
You need to log in before you can comment on or make changes to this bug.