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)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: aleth, Assigned: clokep, Mentored)
References
Details
Attachments
(2 files)
(deleted),
patch
|
nhnt11
:
review+
aleth
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** 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
Assignee | ||
Comment 1•11 years ago
|
||
*** 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.
Reporter | ||
Comment 2•11 years ago
|
||
*** 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.
Assignee | ||
Comment 3•11 years ago
|
||
*** 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
Comment 4•11 years ago
|
||
*** Original post on bio 2156 at 2013-10-24 12:43:49 UTC ***
(In reply to comment #3)
> Thoughts?
Seems reasonable.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [mentor=clokep][good first bug]
Updated•10 years ago
|
Mentor: clokep
Whiteboard: [mentor=clokep][good first bug] → [good first bug]
Assignee | ||
Comment 5•10 years ago
|
||
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #8474144 -
Flags: review?(nhnt11)
Attachment #8474144 -
Flags: review?(aleth)
Comment 6•10 years ago
|
||
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+
Reporter | ||
Comment 7•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 8•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Reporter | ||
Comment 9•10 years ago
|
||
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 → ---
Assignee | ||
Comment 10•10 years ago
|
||
(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
Reporter | ||
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
This reverts the patch AND returns true for SAFELIST to just ignore it.
Attachment #8510014 -
Flags: review?(aleth)
Reporter | ||
Comment 13•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: [good first bug]
Assignee | ||
Comment 14•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•