Skip to content

Conversation

ryanalexander
Copy link

select_known_packs was registering a listener each time the client went into configuration state. Causes issues with Bungeecord.

This fixes people complaining about 1.21.1 bungeecord & velocity issues

@ryanalexander
Copy link
Author

Just noticed this is a duplicate of #1381
Closing

@rom1504
Copy link
Member

rom1504 commented Mar 24, 2025

If you have details on why this is correct or even could add a non regression test it would be useful

@rom1504 rom1504 reopened this Mar 24, 2025
@ryanalexander
Copy link
Author

ryanalexander commented Mar 24, 2025

If you have details on why this is correct or even could add a non regression test it would be useful

"If multiple sperm fertilize an egg (a condition called polyspermy), it typically leads to a non-viable embryo with an abnormal number of chromosomes, usually resulting in the development halting"

Similar occurs with an event listener. If multiple register, the response fires multiple times halting the connection.

--

Non smart-ass answer

The protocol specification means that the pack reply needs to be 1:1 with requests. By registering .on rather than .once, we are causing our method to be called on each emit multiplied by the number of times we have gone into the configuration stage.

While adding non regression testing specifically for this PR would be difficult due to the requirement of having a server change the connection back into configuration stage, this has been manually tested for direct connections to a minecraft vanilla, spigot and paper server, as well as those proxied by Bungeecord and Velocity.

As for why it is correct... There are not really any sources other than reviewing NMS which confirms that sending a resource pack response past the configuration stage will result in a premature socket closure.

@GenerelSchwerz
Copy link

If it helps, there was a discussion on this topic found here. (main discord server) https://discord.gg/YKG6fdKYbB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants