Closed
Bug 962581
Opened 10 years ago
Closed 10 years ago
Notice disconnections faster after waking up from sleep
Categories
(Chat Core :: General, defect)
Chat Core
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: aleth, Assigned: aleth)
References
Details
Attachments
(1 file, 2 obsolete files)
5.78 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
When a PC wakes, it can take a couple of minutes for the ping code in socket.jsm to notice that the connection has been lost, because all timeouts were suspended and continue where they left off. This should be improved, as currently all suspended chats appear to be connected even when they are not after waking.
Assignee | ||
Comment 1•10 years ago
|
||
Feedback on how the different possible states are handled welcome!
Attachment #8363666 -
Flags: review?(clokep)
Attachment #8363666 -
Flags: feedback?(florian)
Comment 2•10 years ago
|
||
Comment on attachment 8363666 [details] [diff] [review] 962581.patch Review of attachment 8363666 [details] [diff] [review]: ----------------------------------------------------------------- I think what you are trying to do makes sense. I haven't done a real review though (you only requested feedback). ::: chat/modules/socket.jsm @@ +324,5 @@ > + // or if there never was any activity before we went to sleep, > + // time out immediately. > + if (!this._lastAliveTime || > + (this._disconnectTimer && elapsedTime > 30000) || > + elapsedTime > 300000) These 2 hard coded values really want to be consts. This looks too much like they wanted to be the same value and you typoed (missed a 0) for one of them.
Attachment #8363666 -
Flags: feedback?(florian) → feedback+
Comment 3•10 years ago
|
||
Comment on attachment 8363666 [details] [diff] [review] 962581.patch Review of attachment 8363666 [details] [diff] [review]: ----------------------------------------------------------------- Overall it looks reasonable, I had a few comments that don't necessary mean something needs to be changed. ::: chat/modules/socket.jsm @@ +141,5 @@ > connect: function(aHost, aPort, aSecurity, aProxy) { > if (Services.io.offline) > throw Cr.NS_ERROR_FAILURE; > > + Services.obs.addObserver(this, "wake_notification", false); Should we have a comment that this only works for Mac/Windows with a bug reference? @@ +324,5 @@ > + // or if there never was any activity before we went to sleep, > + // time out immediately. > + if (!this._lastAliveTime || > + (this._disconnectTimer && elapsedTime > 30000) || > + elapsedTime > 300000) It probably should be used at http://mxr.mozilla.org/comm-central/source/chat/modules/socket.jsm#613 too. Can you reorder these comments to be the order you check things in? The 5 minutes seems kind of arbitrary...I (for some reason) feel like it should match the 2 minute ping timer we use above. @@ +326,5 @@ > + if (!this._lastAliveTime || > + (this._disconnectTimer && elapsedTime > 30000) || > + elapsedTime > 300000) > + this.onConnectionTimedOut(); > + else if (this._pingTimer && !this._disconnectTimer) { When does this happen? @@ +416,5 @@ > > // Remove the handled data. > this._incomingDataBuffer.splice(0, size); > + > + this._lastAliveTime = Date.now(); Can we combine this with the call in _handleQueue so it's not in the if statement for binary protocols?
Attachment #8363666 -
Flags: review?(clokep)
Attachment #8363666 -
Flags: feedback+
Assignee | ||
Comment 4•10 years ago
|
||
Addresses review comments (sometimes just with (hopefully) improved comments in the code).
Attachment #8363666 -
Attachment is obsolete: true
Attachment #8364287 -
Flags: review?(clokep)
Comment 5•10 years ago
|
||
Comment on attachment 8364287 [details] [diff] [review] 962581.patch 2 Review of attachment 8364287 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks fine. I find the logic a bit confusing about when we disconnect vs. send a ping, but aleth explained it to me on IRC and I had forgotten a couple details about how we send pings. I can't think of any ways to improve the comments though, so let's give this a try.
Attachment #8364287 -
Flags: review?(clokep) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Simplified if statement as discussed on IRC, carrying r+ forward.
Attachment #8364287 -
Attachment is obsolete: true
Attachment #8364293 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Whiteboard: checkin-needed
Comment 7•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/df0b77fa9170
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: checkin-needed
Updated•10 years ago
|
Target Milestone: --- → 1.6
You need to log in
before you can comment on or make changes to this bug.
Description
•