Closed Bug 955595 Opened 11 years ago Closed 10 years ago

Figure out how to handle ISUPPORT info for LIST

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: clokep, Mentored)

References

Details

Attachments

(2 files)

*** Original post on bio 2156 at 2013-09-06 12:00:00 UTC *** Decide what to do with iSUPPORT ELIST and SAFELIST http://www.irc.org/tech_docs/005.html
Blocks: 955503
*** Original post on bio 2156 at 2013-10-11 19:21:32 UTC *** SAFELIST tells us you will not be kicked for requesting LIST, the conservative thing to do would be to not automatically send LIST unless we've received SAFELIST (which not breaking /list, by the way). I don't think ELIST is useful unless the UI supports a way to query based on different things...but even then this isn't really useful because we're requesting everything from the server and caching it, we're not querying the server every time.
*** Original post on bio 2156 at 2013-10-12 15:24:05 UTC *** Do we want to be conservative? We should probably check if most of the major networks at least actually provide ISUPPORT SAFELIST.
*** Original post on bio 2156 at 2013-10-24 10:48:53 UTC *** I think what I'd like to see is the following: - Change chat.irc.automaticList to be an integer, it will have three values (0, 1, 2), default it to 1: 0: Never use list. 1: Use list if the server reports safelist. 2: Always use list. - Change the code around [1] to check this preference + whether the 005 supports SAFELIST or not (so pretty much cache this value at [2]). Thoughts? [1] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#933 [2] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircISUPPORT.jsm#186
*** Original post on bio 2156 at 2013-10-24 12:43:49 UTC *** (In reply to comment #3) > Thoughts? Seems reasonable.
Whiteboard: [mentor=clokep][good first bug]
Mentor: clokep
Whiteboard: [mentor=clokep][good first bug] → [good first bug]
Attached patch Patch v1 (deleted) — Splinter Review
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #8474144 - Flags: review?(nhnt11)
Attachment #8474144 - Flags: review?(aleth)
Comment on attachment 8474144 [details] [diff] [review] Patch v1 Review of attachment 8474144 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks!
Attachment #8474144 - Flags: review?(nhnt11) → review+
Comment on attachment 8474144 [details] [diff] [review] Patch v1 Review of attachment 8474144 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I do hope all standard networks send SAFELIST...
Attachment #8474144 - Flags: review?(aleth) → review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
I believe we've made a mistake here and SAFELIST doesn't mean what we thought it did. I noticed that we weren't getting any LIST results on moznet due to the server not sending SAFELIST. The description of SAFELIST in the link above is "The LIST is sent in multiple iterations so send queue won't fill and kill the client connection." Another description, in an older version of inspircd documentation, says [1] "This token indicates that channel listing is throttled so that it will not flood your client off the server." So it appears SAFELIST does *not* tell you that you're not going to be kicked for LIST - it tells you that the server is going to try very hard not to flood *the client*. Therefore I think we should back out this patch as it does the wrong thing. To check further, I asked on #inspircd: 18:19:15 - aleth: Hi! I was wondering about the status of SAFELIST. The module m_safelist seems to have disappeared, what's the story behind that? 18:19:46 - aleth: Also a heads-up that the chatspike.net SSL cert has expired 18:59:52 - SaberUK: aleth: its not needed anymore due to internal changes 19:00:55 - aleth: SaberUK: Thanks. Just to confirm, the original purpose of the ISUPPORT flag was to tell clients they wouldn't be flooded for LIST, not that they wouldn't be kicked for LIST? 19:02:54 - SaberUK: aleth: i cant remember 19:03:29 - SaberUK: it was removed like five years ago So it seems we are conditioning whether we automatically list or not on a flag provided by an obsolete module that hasn't been around for years. [1] http://webcache.googleusercontent.com/search?q=cache:0Ifx2TktOocJ:https://wiki.inspircd.org/RPL_ISUPPORT_Tokens+&cd=1&hl=en&ct=clnk
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to aleth [:aleth] from comment #9) > "The LIST is sent in multiple iterations so send queue won't fill and kill > the client connection." I read this as "The send queue won't be filling forcing us to disconnect the client." Perhaps that's not what it actually means, however. [1] seems to match what I think this means, for what it's worth. Obviously those guys should know what this does more than we do though... I'm OK backing this out, but I'm confused at what this flag actually means... [1] http://tools.ietf.org/html/draft-brocklesby-irc-isupport-03#section-3.15
(In reply to Patrick Cloke [:clokep] from comment #10) > (In reply to aleth [:aleth] from comment #9) > > "The LIST is sent in multiple iterations so send queue won't fill and kill > > the client connection." > > I read this as "The send queue won't be filling forcing us to disconnect the > client." Perhaps that's not what it actually means, however. [1] seems to > match what I think this means, for what it's worth. > > Obviously those guys should know what this does more than we do though... > I'm OK backing this out, but I'm confused at what this flag actually means... It's definitely confusing, and that's the way I read it too, originally. Either way we should back it out as fully updated servers which don't have any flood problem no longer send SAFELIST.
Blocks: 1082501
Attached patch Revert Patch v1 (deleted) — Splinter Review
This reverts the patch AND returns true for SAFELIST to just ignore it.
Attachment #8510014 - Flags: review?(aleth)
Comment on attachment 8510014 [details] [diff] [review] Revert Patch v1 Review of attachment 8510014 [details] [diff] [review]: ----------------------------------------------------------------- Ah, good idea.
Attachment #8510014 - Flags: review?(aleth) → review+
Keywords: checkin-needed
Whiteboard: [good first bug]
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: