-
-
Notifications
You must be signed in to change notification settings - Fork 403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools.target: don't clobber existing permissions in Channel.add_user()
#2563
Conversation
Centralizing this removes a possible future pitfall: Updating what is sent in response to the bot joining a new channel in one location but not in another. This better positions us to conditionally decide (based on e.g. enabled capabilities or ISUPPORT tokens) what to request from the server.
Test case slightly modified from the one in the relevant bug report. Reporter gets a co-authorship for providing it, as it was very helpful. Co-authored-by: half-duplex <[email protected]>
Ah, well, maybe this isn't a good solution. It depends on whether the behavior of |
add_user is called in three places: JOIN, RPL_NAMREPLY, and _record_who(). Could add a clear=False to add_user and call it with clear=True from JOIN? Idk, all of these feel gross when NAMREPLY and WHOREPLY should be absolute and trustworthy. I guess whether we do something like this or consider it an Unreal bug depends on whether it's likely to happen elsewhere. The possibility of different threads — or even servers with link latency — answering things makes it feel not entirely unlikely. The network where I originally observed this has several servers. |
And that's why I initially didn't even plan to work on this. The patch here was an experiment, and the only reasonable way to put it "out there" for discussion was to open a PR. 🤷♂️
That could be an interesting line of discussion, though I think it's less about whether to If we can reasonably trust NAMREPLY/WHOREPLY for users previously in the channel when Sopel joins, and a race condition is only a concern for users who join after Sopel, we can possibly develop sensible logic. But it all still comes down to whether we can trust the server's NAMREPLY/WHOREPLY data, and none of this overthinking should be needed because ultimately, what the server says is supposed to be trustworthy. (If I come off as frustrated, it's because I am. Inability to trust state expressed in the server's responses could break a lot more than privilege tracking.) |
Set a flag for "we have a WHO out" on the User object, and if anything weird (MODE) happens before ENDOFWHO for it, re-WHO? |
Hmm, I'm going to take this discussion back to the original issue. This doesn't seem to be the way forward. |
Description
Here's a potential fix for #2562, including a slightly modified version of the test case @half-duplex wrote for that issue.
The race-condition feeling I get from the bug report is a strong one. I can only assume that the IRCd on which this was reported (Unreal 3.2, which is admittedly EOL) suffers from a race condition between its
who
andmode
modules, something like:#channel
#channel
members, including Sopel and ChanServwho
module finds the user, builds the status prefix, and sends the reply to Sopelmode
module (possibly on a different server in the network, adding more delay) processes (and possibly has to relay) the MODE change sent by ChanServLots of educated guesses there based on skimming the C source for Unreal 3.2, but even if the exact sequence is wrong the result walks/quacks like a race condition, so we have to treat it like one.
I've also bundled a tweak to
coretasks
, moving the duplicateMODE
andWHO
invocations for a new channel to a single location (new helper function) and adding some more commentary on the join-queue logic, simply because those tweaks don't warrant a pull request of their own.Checklist
make qa
(runsmake lint
andmake test
)Notes
No milestone up front; I'm not positive that we should shoehorn this particular issue & patch into 8.0.0. Depends on the reaction from other maintainers whether it goes in 8.0.0, 8.0.1, or 8.1.0. 😉