Closed Bug 954843 Opened 8 years ago Closed 8 years ago

Handle (dis)connects better in IRC

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

(Whiteboard: [1.2-blocking])

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1409 at 2012-04-28 14:50:00 UTC ***

See [1] for a conversation flo and I had about ways it can be improved.

In summary, we need to:
 1. Add a check to the quit method [2] to not send anything if we're not connected.
 2. If there's no socket, we should immediately call gotDisconnected
 3. sendMessage or sendRawMessage should refused to send things (error? call gotDisconnected?) if there's no socket
 4. Remove the reconnect method in the socket code (since it's unused)

[1] http://log.bezut.info/instantbird/120428/#m100
[2] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#808
*** Original post on bio 1409 at 2012-05-10 20:01:21 UTC ***

5. Fix the code of the "ERROR" handler (http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircBase.jsm#211) to mark the account as disconnected with an error code that will cause automatic reconnection if we receive an ERROR without having sent a QUIT command before.
Whiteboard: [1.2-blocking]
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1409 as attmnt 1460 at 2012-05-11 02:17:00 UTC ***

I'm not sure this meets all the requirements of what we discussed, but I think it does.
Attachment #8353213 - Flags: feedback?(florian)
*** Original post on bio 1409 at 2012-05-11 02:17:35 UTC ***

aleth, I'd appreciate if you could take a look at this too. :)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353213 [details] [diff] [review]
Patch v1

*** Original change on bio 1409 attmnt 1460 at 2012-05-11 10:23:20 UTC ***


>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>--- a/chat/protocols/irc/irc.js
>+++ b/chat/protocols/irc/irc.js
>@@ -846,8 +846,10 @@ ircAccount.prototype = {
>   _quitTimer: null,
>   // RFC 2812 Section 3.1.7.
>   quit: function(aMessage) {
>-    this.sendMessage("QUIT",
>-                     aMessage || this.getString("quitmsg") || undefined);
>+    if (this._socket.isAlive()) {
>+      this.sendMessage("QUIT",
>+                       aMessage || this.getString("quitmsg") || undefined);
>+    }
>   },

Can this be called if this._socket is null?

I think you also somehow need a check in the disconnect method, otherwise I don't see what would call gotDisconnected immediately (without the 2 seconds timer) when we are already offline because the network is offline.

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>--- a/chat/protocols/irc/ircBase.jsm
>+++ b/chat/protocols/irc/ircBase.jsm
>@@ -211,9 +211,17 @@ var ircBase = {
>     "ERROR": function(aMessage) {
>       // ERROR <error message>
>       // Client connection has been terminated.
>-      clearTimeout(this._quitTimer);
>-      this.gotDisconnected(Ci.prplIAccount.NO_ERROR,
>-                           aMessage.params[0]); // Notify account manager.
>+      if (!this.disconnecting || !this.disconnected) {

When do you expect this to be called with this.disconnected = true?

>+        // We received an ERROR message when we weren't expecting it, this is
>+        // probably the server giving us a ping timeout.
>+        this.gotDisconnected(Ci.prplIAccount.ERROR_NETWORK_ERROR,
>+                             _("connection.error.lost"));
>+      }
>+      else {
>+        clearTimeout(this._quitTimer);

Shouldn't we also delete this._quitTimer; ?
Attachment #8353213 - Flags: feedback?(florian) → review-
*** Original post on bio 1409 at 2012-05-11 10:35:29 UTC ***

Shouldn't something happen (a this.gotDisconnected call?) if quit() is called and the socket isn't alive? Or can't this case happen? I was thinking of the /quit command in particular. (Of course if the account is already marked as disconnected there is no problem as /quit won't get sent by conversation.xml)
Attached patch Patch v1.3 (obsolete) — Splinter Review
*** Original post on bio 1409 as attmnt 1465 at 2012-05-12 23:12:00 UTC ***

Meets the review comments.
Attachment #8353218 - Flags: review?(florian)
Comment on attachment 8353213 [details] [diff] [review]
Patch v1

*** Original change on bio 1409 attmnt 1460 at 2012-05-12 23:12:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353213 - Attachment is obsolete: true
*** Original post on bio 1409 at 2012-05-13 21:03:14 UTC ***

Comment on attachment 8353218 [details] [diff] [review] (bio-attmnt 1465)
Patch v1.3

I wanted to go ahead an check this in this evening, but I think I have a few additional questions :-(.

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>--- a/chat/protocols/irc/irc.js
>+++ b/chat/protocols/irc/irc.js
>@@ -846,8 +846,10 @@ ircAccount.prototype = {
>   _quitTimer: null,
>   // RFC 2812 Section 3.1.7.
>   quit: function(aMessage) {
>-    this.sendMessage("QUIT",
>-                     aMessage || this.getString("quitmsg") || undefined);
>+    if (this._socket && this._socket.isAlive()) {
>+      this.sendMessage("QUIT",
>+                       aMessage || this.getString("quitmsg") || undefined);
>+    }

In which situation do you expect quit to be called when the socket is inexistent/dead? (I thought we had just disabled commands in conversations of offline accounts)
If you aren't going to send the message, should you call gotDisconnected immediately? (assuming an user calling the /quit command expects the account to become disconnected)

Wouldn't just removing this new test let sendRawMessage handle it? Hmm, or we may want to call it ourselves so that it doesn't start the auto-reconnect timer.

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>--- a/chat/protocols/irc/ircBase.jsm
>+++ b/chat/protocols/irc/ircBase.jsm
>@@ -211,9 +211,18 @@ var ircBase = {
>     "ERROR": function(aMessage) {
>       // ERROR <error message>
>       // Client connection has been terminated.
>-      clearTimeout(this._quitTimer);
>-      this.gotDisconnected(Ci.prplIAccount.NO_ERROR,
>-                           aMessage.params[0]); // Notify account manager.
>+      if (!this.disconnecting) {
>+        // We received an ERROR message when we weren't expecting it, this is
>+        // probably the server giving us a ping timeout.
>+        this.gotDisconnected(Ci.prplIAccount.ERROR_NETWORK_ERROR,
>+                             _("connection.error.lost"));

Shouldn't the unexpected error message received from the server somehow end up in the error console?

>+      }
>+      else {
>+        clearTimeout(this._quitTimer);
>+        delete this._quitTimer;
>+        this.gotDisconnected(Ci.prplIAccount.NO_ERROR,
>+                             aMessage.params[0]); // Notify account manager.

I don't understand why gotDisconnected is called with NO_ERROR *and* an error message. This seems very strange (it was already there before this patch though). Wouldn't calling without parameters do what we want here?
Attached patch Patch v1.4Splinter Review
*** Original post on bio 1409 as attmnt 1466 at 2012-05-13 23:22:00 UTC ***

(In reply to comment #7)
> Comment on attachment 8353218 [details] [diff] [review] (bio-attmnt 1465) [details]
> >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
> >--- a/chat/protocols/irc/irc.js
> >+++ b/chat/protocols/irc/irc.js
> >@@ -846,8 +846,10 @@ ircAccount.prototype = {
> >   _quitTimer: null,
> >   // RFC 2812 Section 3.1.7.
> >   quit: function(aMessage) {
> >-    this.sendMessage("QUIT",
> >-                     aMessage || this.getString("quitmsg") || undefined);
> >+    if (this._socket && this._socket.isAlive()) {
> >+      this.sendMessage("QUIT",
> >+                       aMessage || this.getString("quitmsg") || undefined);
> >+    }
> 
> In which situation do you expect quit to be called when the socket is
> inexistent/dead? (I thought we had just disabled commands in conversations of
> offline accounts)
> If you aren't going to send the message, should you call gotDisconnected
> immediately? (assuming an user calling the /quit command expects the account to
> become disconnected)
I guess we don't, since the command isn't even sent. I tested it without it and everything seemed to work just fine.

> >diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
> >--- a/chat/protocols/irc/ircBase.jsm
> >+++ b/chat/protocols/irc/ircBase.jsm
> >@@ -211,9 +211,18 @@ var ircBase = {
> >     "ERROR": function(aMessage) {
> >       // ERROR <error message>
> >       // Client connection has been terminated.
> >-      clearTimeout(this._quitTimer);
> >-      this.gotDisconnected(Ci.prplIAccount.NO_ERROR,
> >-                           aMessage.params[0]); // Notify account manager.
> >+      if (!this.disconnecting) {
> >+        // We received an ERROR message when we weren't expecting it, this is
> >+        // probably the server giving us a ping timeout.
> >+        this.gotDisconnected(Ci.prplIAccount.ERROR_NETWORK_ERROR,
> >+                             _("connection.error.lost"));
> 
> Shouldn't the unexpected error message received from the server somehow end up
> in the error console?
The expected condition just calls gotDisconnected with no parameters now and we also call ERROR with the received text in the unexpected case.

Thanks for the review. :)
Attachment #8353219 - Flags: review?(florian)
Comment on attachment 8353218 [details] [diff] [review]
Patch v1.3

*** Original change on bio 1409 attmnt 1465 at 2012-05-13 23:22:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353218 - Attachment is obsolete: true
Attachment #8353218 - Flags: review?(florian)
Comment on attachment 8353219 [details] [diff] [review]
Patch v1.4

*** Original change on bio 1409 attmnt 1466 at 2012-05-14 20:46:11 UTC ***

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>--- a/chat/protocols/irc/ircBase.jsm
>+++ b/chat/protocols/irc/ircBase.jsm
>@@ -211,9 +211,21 @@ var ircBase = {
>     "ERROR": function(aMessage) {
>       // ERROR <error message>
>       // Client connection has been terminated.
>-      clearTimeout(this._quitTimer);
>-      this.gotDisconnected(Ci.prplIAccount.NO_ERROR,
>-                           aMessage.params[0]); // Notify account manager.
>+      if (!this.disconnecting) {
>+        // We received an ERROR message when we weren't expecting it, this is
>+        // probably the server giving us a ping timeout.
>+        this.gotDisconnected(Ci.prplIAccount.ERROR_NETWORK_ERROR,
>+                             _("connection.error.lost"));
>+        ERROR("Received unexpected ERROR response:\n" + aMessage.params[0]);

I would prefer this ERROR call before the this.gotDisconnected call (which is likely to (indirectly) cause other messages to be logged to the console).

I'm making this change and removing the trailing whitespace for the check-in. Looks great otherwise, thanks!
Attachment #8353219 - Flags: review?(florian) → review+
*** Original post on bio 1409 at 2012-05-14 21:37:22 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/1afad9aa06d0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Depends on: 954888
You need to log in before you can comment on or make changes to this bug.