Closed
Bug 849364
Opened 12 years ago
Closed 12 years ago
Provide per-websocket way to enable keepalive pings
Categories
(Core :: Networking: WebSockets, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → jduell.mcbugs
Comment 1•12 years ago
|
||
to be clear the proposal is a method on nsIWebsocketChannel, yes?
I think that's fine. I wouldn't want to see us go and making javascript apis without jonas defining it.
but we've got data that shows 60 seconds as the most common NAT timeout period. I think if you put an open ended 55 second heartbeat on b2g you will just crush the battery and consume massive amounts of data. That's likely a dead end.
Comment 2•12 years ago
|
||
Isn't it 30 seconds? According the SPDY_PING_EXPERIMENT_FAIL probe.
Comment 3•12 years ago
|
||
(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.
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
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).
Blocks: simple-push-b2g
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.
No longer blocks: simple-push-b2g
Assignee | ||
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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+
Assignee | ||
Comment 9•12 years ago
|
||
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 #725269 -
Attachment is obsolete: true
Attachment #730862 -
Flags: review?(mcmanus)
Updated•12 years ago
|
Attachment #730862 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
No longer blocks: simple-push-b2g
Blocks: simple-push-b2g
Removing 'blocking 822712' because this doesn't affect the correctness of push in anyway. Filed follow up 855906
No longer blocks: simple-push-b2g
Comment 12•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Comment 13•12 years ago
|
||
we only need to track this if we track bug 855906
tracking-b2g18:
--- → ?
Comment 14•12 years ago
|
||
since we did not track bug 855906, no need to track here.
tracking-b2g18:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•