Closed Bug 962581 Opened 10 years ago Closed 10 years ago

Notice disconnections faster after waking up from sleep

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch 962581.patch (obsolete) — Splinter Review
Feedback on how the different possible states are handled welcome!
Attachment #8363666 - Flags: review?(clokep)
Attachment #8363666 - Flags: feedback?(florian)
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 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+
Attached patch 962581.patch 2 (obsolete) — Splinter Review
Addresses review comments (sometimes just with (hopefully) improved comments in the code).
Attachment #8363666 - Attachment is obsolete: true
Attachment #8364287 - Flags: review?(clokep)
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+
Attached patch 962581.patch 3Splinter Review
Simplified if statement as discussed on IRC, carrying r+ forward.
Attachment #8364287 - Attachment is obsolete: true
Attachment #8364293 - Flags: review+
Whiteboard: checkin-needed
https://hg.mozilla.org/comm-central/rev/df0b77fa9170
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: checkin-needed
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.