Closed Bug 955667 Opened 10 years ago Closed 10 years ago

Unhandled IRC message 263: LIST refused

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 2220 at 2013-10-15 16:49:00 UTC ***

Timestamp: 10/15/2013 06:43:24 PM
Warning: Unhandled IRC message:
{"rawMessage":":dickson.freenode.net 263 aleth LIST :This command could not be completed because it has been used recently, and is rate-limited.","command":"263","params":["aleth","LIST","This command could not be completed because it has been used recently, and is rate-limited."],"servername":"dickson.freenode.net"}
Source File: resource://gre/components/irc.js
Line: 692
Source Code:
prpl-irc

We should handle this response to LIST.
Blocks: 955013
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2220 as attmnt 2950 at 2013-10-15 18:36:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354731 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
*** Original post on bio 2220 at 2013-10-15 18:38:02 UTC ***

nhnt11: Is there a way to tell the callback receivers that LIST failed?
Comment on attachment 8354731 [details] [diff] [review]
Patch

*** Original change on bio 2220 attmnt 2950 at 2013-10-15 18:51:20 UTC ***

Can you add a comment about when this happens for list or something. I doubt you want to do the server message always for list, by the way. If it was from a command we probably want to print out a message, otherwise...I'm not sure what to do.
Attachment #8354731 - Flags: review?(clokep) → review-
*** Original post on bio 2220 at 2013-10-15 18:54:16 UTC ***

(In reply to comment #3)
> I doubt you want to do the server message always for list, by the way. If it 
> was from a command we probably want to print out a message, otherwise...I'm 
> not sure what to do.

How soon after receiving this LIST failure message do you think we should retry? (nhnt11 thinks 12h is too long in this case).
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2220 as attmnt 2952 at 2013-10-15 19:59:00 UTC ***

Improve comment, set last list time in such a way that we may retry after an hour rather than 12.
Attachment #8354733 - Flags: review?(clokep)
Comment on attachment 8354731 [details] [diff] [review]
Patch

*** Original change on bio 2220 attmnt 2950 at 2013-10-15 19:59:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354731 - Attachment is obsolete: true
Comment on attachment 8354733 [details] [diff] [review]
Patch

*** Original change on bio 2220 attmnt 2952 at 2013-10-15 20:07:44 UTC ***

This is pretty clearly untested.

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>     "263": function(aMessage) { // RPL_TRYAGAIN
>       // <command> :Please wait a while and try again.
>-      // TODO setTimeout for a minute or so and try again?
>-      return false;
>+      if (aMessage.params[1] == "LIST" && this._pendingList) {
What does pendingList mean in this context?

>+        // We may receive this from servers which rate-limit LIST if the
>+        // server believes us to be asking for LIST data to soon after the
Typo: "too soon"

>+        // previous request.
>+        // Tidy up as we won't be receiving any more channels.
>+        this._sendRemainingRoomInfo();
>+        // Fake the last LIST time so that we may try again in an hour.
Be explicit and say "one hour" please.

>+      return serverMessage(this, aMessage);
Should this return false as previously?

>diff --git a/chat/protocols/irc/ircUtils.jsm b/chat/protocols/irc/ircUtils.jsm
>+const kListRefreshInterval: 12 * 60 * 60 * 1000; // 12 hours.
I think you mean =, not :. This needs to be exported if you want access to it in other places. This should probably have a comment above it also.
Attachment #8354733 - Flags: review?(clokep) → review-
*** Original post on bio 2220 at 2013-10-15 20:15:42 UTC ***

>>+ if (aMessage.params[1] == "LIST" && this._pendingList) { 
> What does pendingList mean in this context? 

It means that the user has used /list or the newtab service has sent
LIST. Therefore there are callbacks etc we need to tidy up.

>>+ // Fake the last LIST time so that we may try again in an hour. 
> Be explicit and say "one hour" please. 

No, it's not going to happen again in one hour automatically. It'll only
happen if after one hour
someone opens a newtab...

>>+ return serverMessage(this, aMessage); 
> Should this return false as previously? 

I think showing it in the server tab is better than not handling it at
all.

>>+const kListRefreshInterval: 12 * 60 * 60 * 1000; // 12 hours. 

Thanks for catching this! The patch was untested as freenode is suddenly
allowing me to LIST again... not a good excuse ;)
Attached patch PatchSplinter Review
*** Original post on bio 2220 as attmnt 2954 at 2013-10-15 20:33:00 UTC ***

Should address the review comments. Tested in the "doesn't break IRC" sense.
Attachment #8354735 - Flags: review?(clokep)
Comment on attachment 8354733 [details] [diff] [review]
Patch

*** Original change on bio 2220 attmnt 2952 at 2013-10-15 20:33:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354733 - Attachment is obsolete: true
Comment on attachment 8354735 [details] [diff] [review]
Patch

*** Original change on bio 2220 attmnt 2954 at 2013-10-16 13:06:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354735 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2220 at 2013-10-18 11:14:23 UTC ***

http://hg.instantbird.org/instantbird/rev/5ae435057c7a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.