Closed Bug 849364 Opened 7 years ago Closed 7 years ago
Provide per-websocket way to enable keepalive pings
websocket pings are turned off by default (we were worried they consume too much power--ie. wake up the phone and/or radio when it would otherwise be asleep. It's not actually clear if that's the case on android or B2G). But in any case bug 763198 is simplified by turning keepalive on for its websocket connection, so this will provide a way to do that per-websocket.
Isn't it 30 seconds? According the SPDY_PING_EXPERIMENT_FAIL probe.
(In reply to Honza Bambas (:mayhemer) from comment #2) > Isn't it 30 seconds? According the SPDY_PING_EXPERIMENT_FAIL probe. I'm pretty sure it was 60. We've removed that telem a while ago, so any data points now on the dashboard are stragglers from a few old browsers without much representation, right? There definitely was a lot of variation though. in any event - 30 would be even twice as bad for the purposes of this bug.
Metrics doesn't say how many submissions for the one probe I want to examine has been submitted for a date. Or I don't know how to get that info.
Patrick, you might be right, but from the IRC chat I posted in bug 849377 I couldn't get a clear answer as to how bad the power issue is. And there were noises about doing the pings less frequently when the screen is off. Though if that just results in the websocket connection being killed (and the app woken up with onstop() and then just reopening a new WS, then it's not a battery win. Would we be better off requiring the app to do the ping here? That way it would presumably only happen when the app is awake (not sure when that is for B2G push--it's part of firefox OS, not a regular app).
B2G push always keep a websocket awake. The only time the socket is off is if nobody has signed up for Push. The 'optimization' in case of certain carriers is that they can wake up the service over UDP, in which case the WebSocket is switched off. But over wi-fi, always on.
Even simple stuff gets complicated with e10s :) Do we need to clamp() SetPingTimeout values as we do when we read them from prefs? I wasn't clear on why we're clamping to that particular range. https://tbpl.mozilla.org/?tree=Try&rev=9a67e3e6eb03
Attachment #725269 - Flags: review?(mcmanus)
Comment on attachment 725269 [details] [diff] [review] v1 Review of attachment 725269 [details] [diff] [review]: ----------------------------------------------------------------- Sorry this is so much code for something that is already implemented :( The clamp is probably ok to skip from chrome.
Attachment #725269 - Flags: review?(mcmanus) → review+
Testing v1 exposed race conditions. Checked and we can get away with simple fix--just allow setting ping timeout values *before* asyncOpen is called. Tested by hand the 4-space of (e10s or not, call before/after asyncOpen). Automated test not possible w/current websockets test infrastructure.
Attachment #730862 - Flags: review?(mcmanus) → review+
Removing 'blocking 822712' because this doesn't affect the correctness of push in anyway. Filed follow up 855906
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.