Skip to content

PlayerLoginEvent fires twice #8676

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.

Sign up for GitHub

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

Jump to bottom
Open
TheMysterys opened this issue Dec 15, 2022 · 5 comments · May be fixed by #12323
Open

PlayerLoginEvent fires twice #8676

TheMysterys opened this issue Dec 15, 2022 · 5 comments · May be fixed by #12323
Labels
status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. type: bug Something doesn't work as it was intended to.

Comments

@TheMysterys
Copy link

TheMysterys commented Dec 15, 2022 •
edited
Loading

Expected behavior

When a player connects, the event should only fire once

Observed/Actual behavior

The event fires twice

Steps/models to reproduce

Add the below event listener to your plugin (This event listener can be replaced by turning on the whitelist and looking at the "Disconnecting" logs. The addition of code just makes it a bit easier to see)

@EventHandler
public void playerLoginEvent (PlayerLoginEvent event) {
     System.out.println("Testing");
}

Join on an account and "Testing" will be logged twice

Plugin and Datapack List

[22:58:48 INFO]: Plugins (3): Campfire, LuckPerms, spark

Paper version

[22:58:00 INFO]: This server is running Paper version git-Paper-333 (MC: 1.19.3) (Implementing API version 1.19.3-R0.1-SNAPSHOT) (Git: eec64a4)
You are running the latest version

Other

No response
@TheMysterys TheMysterys added status: needs triage type: bug Something doesn't work as it was intended to. labels Dec 15, 2022
@Machine-Maker Machine-Maker added status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. version: 1.19 Game version 1.19 and removed status: needs triage labels Dec 15, 2022
@Machine-Maker
Copy link
Member

As far as I can tell, when the Connection is first disconnected, the channel is requested to close, but upstream removed the "await" part of that close, so the next tick happens and the channel isn't fully closed which makes the player's login be attempted to be handled again.

@github-project-automation github-project-automation bot moved this to ✅ Accepted in Issues: Bugs Apr 6, 2023
@Gameoholic
Copy link
Contributor

I can't seem to reproduce this, was this fixed already?

This server is running Paper version git-Paper-170 (MC: 1.20.1) (Implementing API version 1.20.1-R0.1-SNAPSHOT) (Git: 39953cf)
You are running the latest version
Previous version: git-Paper-169 (MC: 1.20.1)

@Warriorrrr
Copy link
Member

I can still reproduce this, was also able to get it to fire 3 times sometimes

@Warriorrrr Warriorrrr added version: 1.20 and removed version: 1.19 Game version 1.19 labels Sep 15, 2023
@Warriorrrr
Copy link
Member

I can't find exactly when this got fixed, but we now check if the connection is still open before going through this logic, which fixes this issue.

@github-project-automation github-project-automation bot moved this from ✅ Accepted to Done in Issues: Bugs Jan 25, 2025
@emilyy-dev
Copy link
Member

I can still reproduce this issue

I can't find exactly when this got fixed, but we now check if the connection is still open before going through this logic, which fixes this issue.

iirc that was there before. The problem resides
here

Paper/paper-server/patches/sources/net/minecraft/network/Connection.java.patch

Lines 238 to 249 in bb1beda

public void disconnect(DisconnectionDetails disconnectionDetails) {
+ this.preparing = false; // Spigot
if (this.channel == null) {
this.delayedDisconnect = disconnectionDetails;
}
if (this.isConnected()) {
- this.channel.close().awaitUninterruptibly();
+ this.channel.close(); // We can't wait as this may be called from an event loop.
this.disconnectionDetails = disconnectionDetails;
}
}

and (actually where the event is fired)

Paper/paper-server/patches/sources/net/minecraft/server/network/ServerLoginPacketListenerImpl.java.patch

Lines 70 to 72 in bb1beda

+ if (this.connection.isConnected()) { // Paper - prevent logins to be processed even though disconnect was called
this.verifyLoginAndFinishConnectionSetup(Objects.requireNonNull(this.authenticatedProfile));
- }

The isConnected checks if the channel is still open, but since this part of the process runs on the server thread, Channel#close needs to jump to the netty event loop. No clue why this would take multiple tick-times to process, especially on a simple test server, but I guess it can. Simple solution would be to gate disconnection behind a CAS and have isConnected check that but, bleh

@emilyy-dev emilyy-dev reopened this Mar 21, 2025
@papermc-projects papermc-projects bot moved this from Done to ✅ Accepted in Issues: Bugs Mar 21, 2025
@orang3i orang3i linked a pull request Mar 21, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. type: bug Something doesn't work as it was intended to.
Projects
Issues: Bugs
Status: ✅ Accepted
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix PlayerLoginEvent firing multiple times. orang3i/Paper
6 participants
@kashike @Machine-Maker @Gameoholic @emilyy-dev @TheMysterys @Warriorrrr