Closed Bug 955245 Opened 10 years ago Closed 10 years ago

IRC accounts should timeout when the connection to the server has stalled

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1812 at 2012-11-20 00:29:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1812 as attmnt 2108 at 2012-11-20 00:29:00 UTC ***

+++ This bug was initially created as a clone of Bug #955221 (bio 1789) +++

I think we should both use a read/write timeout at the socket.jsm level for IRC connections, and use IRC PING commands to keep the connection alive. This should help with some issues like what florian has seen.

I propose that we do:
- socket.jsm timeout of 5 minutes (just to be safe; that should never actually happen).
- send an PING command every 2 minutes if no data has been received.
- disconnect and auto-reconnect the account 30s after sending a PING if no reply arrives.

Note that the patch inf bug 955221 (bio 1789) doesn't seem to take into account the "if no data has been received"?

This patch generalizes the code from bug 955221 (bio 1789) and applies and IRC version of it.
Attachment #8353869 - Flags: review?(florian)
*** Original post on bio 1812 at 2012-11-20 00:30:00 UTC ***

Doh, attachment 8353869 [details] [diff] [review] (bio-attmnt 2108) has the wrong bug # in it, it should be 1812, not 1811. Sorry about that. :(
*** Original post on bio 1812 at 2012-11-20 09:55:49 UTC ***

(In reply to comment #0)

> Note that the patch inf bug 955221 (bio 1789) doesn't seem to take into account the "if no
> data has been received"?

See http://lxr.instantbird.org/instantbird/source/chat/protocols/xmpp/xmpp-session.jsm#200
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1812 as attmnt 2114 at 2012-11-21 16:33:00 UTC ***

This will now not send a ping until data has not been received for the ping timeout.
Attachment #8353875 - Flags: review?(florian)
Comment on attachment 8353869 [details] [diff] [review]
Patch

*** Original change on bio 1812 attmnt 2108 at 2012-11-21 16:33:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353869 - Attachment is obsolete: true
Attachment #8353869 - Flags: review?(florian)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353875 [details] [diff] [review]
Patch v2

*** Original change on bio 1812 attmnt 2114 at 2012-12-12 15:49:46 UTC ***

Florian r-ed this over IRC for needing better documentation in general throughout socket.jsm:

9:03:26 AM - flo-retina: I guess I'm just not fully convinced that this is something we really need at the socket.jsm level, rather than at the prpl level
9:03:46 AM * clokep_work was attempting to share code.
9:04:15 AM - flo-retina: yeah, it does reduce code duplication a bit, but at the cost of making a kinda generic API much less generic
9:04:52 AM - clokep_work: I don't really see how it is "much less generic". I see it as adding optional functionality.
9:05:30 AM - flo-retina: maybe my comment is just that we need much better documentation in socket.jsm about how that code is supposed to be used
9:06:12 AM - clokep_work: That...I definitely agree with.
9:06:18 AM - clokep_work: Writing documentation isn't fun. :(
9:06:28 AM - clokep_work: socket.jsm is fairly generic though, it doesn't assume it is in an account or anything.
9:06:43 AM - flo-retina: right
9:06:51 AM - flo-retina: (some accounts could need more than one socket anyway)
9:06:57 AM - clokep_work: Yup.
9:08:49 AM - clokep_work: I just see sending a keep alive as something that wouldn't be uncommon when using a socket.
9:13:33 AM - flo-retina: but the 'API' says "ping" ;)
9:15:28 AM - clokep_work: If that's your only issue we can rename it keepalive.
9:15:49 AM - flo-retina: no, r= "needs documentation" ;)
9:16:48 AM - flo-retina: s/=/-/
9:17:33 AM - clokep_work: Then r- it and I'll add documentation.
9:17:42 AM - clokep_work: That's a perfectly reasonable review comment.
9:17:48 AM - clokep_work: I can also change it to say keepAlive while doing that. ;)
9:18:04 AM - flo-retina: the documentation can say that
9:18:23 AM - flo-retina: and maybe "ping" is the right word for a keep alive packet that expects a reply
Attachment #8353875 - Flags: review?(florian) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
*** Original post on bio 1812 as attmnt 2175 at 2012-12-20 00:38:00 UTC ***

This includes a bit more documentation. If you think more is needed, please provide more guidance as to what needs more information, thanks!
Attachment #8353937 - Flags: review?(florian)
Comment on attachment 8353875 [details] [diff] [review]
Patch v2

*** Original change on bio 1812 attmnt 2114 at 2012-12-20 00:38:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353875 - Attachment is obsolete: true
Comment on attachment 8353937 [details] [diff] [review]
Patch v3

*** Original change on bio 1812 attmnt 2175 at 2012-12-20 06:39:35 UTC ***

- You forgot "sendPing" in the list of methods the consumer can/should reimplement.

- It should be obvious when reading the documentation that the ping feature of socket.jsm is optional. "This should be called whenever a message is received" makes it seem like it's mandatory to use it.
Attachment #8353937 - Flags: review?(florian) → review-
*** Original post on bio 1812 at 2012-12-20 12:32:15 UTC ***

Comment on attachment 8353937 [details] [diff] [review] (bio-attmnt 2175)
Patch v3

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>+    // We've received data and are past the authentication stage.
>+    if (this.connected)
>+      this.resetPingTimer();

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>@@ -288,21 +288,28 @@ var ircBase = {
>+    "PONG": function(aMessage) {
>+      // PONG <server> [ <server2> ]
>+      // Ping to keep the connection alive.
>+      this._socket.cancelDisconnectTimer();
>+      this._socket.resetPingTimer();
The ping timer should already have been reset (whenever we receive data), so we don't need to reset it here.
Attached patch Patch v4Splinter Review
*** Original post on bio 1812 as attmnt 2178 at 2012-12-21 01:04:00 UTC ***

Much more extensive documentation.
Attachment #8353940 - Flags: review?(florian)
Comment on attachment 8353937 [details] [diff] [review]
Patch v3

*** Original change on bio 1812 attmnt 2175 at 2012-12-21 01:04:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353937 - Attachment is obsolete: true
Comment on attachment 8353940 [details] [diff] [review]
Patch v4

*** Original change on bio 1812 attmnt 2178 at 2012-12-21 16:01:45 UTC ***

>+  // If sing the ping functionality, this should be called whenever a message is

Typo here. I'll fix it before the check-in.

Thanks for the improved documentation! :-)
Attachment #8353940 - Flags: review?(florian) → review+
*** Original post on bio 1812 at 2012-12-23 00:11:19 UTC ***

http://hg.instantbird.org/instantbird/rev/6cfe109b1d9f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.