Skip to content
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

coretasks: join channels on MOTD #2321

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Jul 14, 2022

Description

Tin, should fix #2289 for good but I didn't test that section in particular.

This was way too easy. The tests didn't even fail, everything worked on the first try. I don't really complain, I'm just... yeah, OK, all good then.

I've tested it with the Libera servers (they use solanum-1.0-dev), and with a friend's server using UnrealIRCd-5.2.4. Both are fairly modern IRC servers, so it is possible that this breaks with older server that don't follow the MOTD specification.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@Exirel Exirel added Bugfix Generally, PRs that reference (and fix) one or more issue(s) Core/IRC Protocol Handling labels Jul 14, 2022
@Exirel Exirel added this to the 8.0.0 milestone Jul 14, 2022
@Exirel Exirel mentioned this pull request Jul 14, 2022
@half-duplex
Copy link
Member

half-duplex commented Jul 14, 2022

This fixes the specific issues I was observing in #2289. I haven't poked into the state graph - is this still a race, just one we more reliably win? Or do we know that we'll never get an ENDOFMOTD/NOMOTD until after the UHNAMES has been received?

@dgw
Copy link
Member

dgw commented Jul 24, 2022

Horse docs says that:

Upon successful completion of the registration process, the server MUST send, in this order, the RPL_WELCOME (001), RPL_YOURHOST (002), RPL_CREATED (003), RPL_MYINFO (004), and at least one RPL_ISUPPORT (005) numeric to the client.

The UHNAMES flag, if advertised, will appear in a 005 line, at which point Sopel will immediately send the PROTOCTL to enable it.

Some other stuff "SHOULD" or "MAY" happen before the server again "MUST" respond as if the client sent it the MOTD command, which will trigger the start of channel joins as of this patch. By that time, since coretasks stuff tends to use plugin.thread(False) to guard against processing protocol events in parallel, PROTOCTL UHNAMES should have taken effect.

Is it technically still a race? Yeah, I guess. But by the time Sopel processes everything the server has sent and starts asking to join channels, a race should be practically impossible because disabling threaded handling forces everything the server sends after registration to be processed in sequence.

@Exirel
Copy link
Contributor Author

Exirel commented Jul 24, 2022

So, is that an approval?

@dgw
Copy link
Member

dgw commented Jul 24, 2022

Ask @half-duplex, as the original reporter of #2289.

Copy link
Member

@half-duplex half-duplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if there's still potentially a race, between the try/catch, this, and that bad results don't actually matter, I'm fine calling it resolved.

@dgw dgw merged commit 20f8c51 into sopel-irc:master Jul 25, 2022
@Exirel Exirel deleted the join-channels-on-modt branch April 8, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) Core/IRC Protocol Handling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UHNAMES Race Condition
3 participants