`ConnectedPlayer#disconnected` called even when connection fails due to duplicate connection
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 2.3k
- Forks
- 959
- Avg merge
- 5d 10h
- Merged PRs (30d)
- 2
Description
Expected Behavior
This method should'nt be called.
Actual Behavior
This method does get called, possibly resulting in undesired VelocityBossBarImplementation#viewerDisconnected calls.
For reference, the entire ConnectedPlayer#disconnected method:
public void disconnected() {
for (final VelocityBossBarImplementation bar : this.bossBars) {
bar.viewerDisconnected(this);
}
}
Steps to Reproduce
Add a printline/log statement in ConnectedPlayer#disconnected, join the proxy with minecraft account A, try to re-join the proxy with the same account.
The second connection will fail, and ConnectedPlayer#disconnected still gets called.
Plugin List
n/a
Velocity Version
Compiled from latest commit e0db25664fc82eabd9fde5aac22a2311a9765975 + the log statement in disconnected().
Additional Information
This currently isn't an issue, as bossbars won't be registered to the ConnectedPlayer. However if more logic ever gets added to this method this might be an issue.
My proposed fix is to gate the connection.disconnected() call in VelocityServer#unregisterConnection:
public void unregisterConnection(ConnectedPlayer connection) {
connectionsByName.remove(connection.getUsername().toLowerCase(Locale.US), connection);
connectionsByUuid.remove(connection.getUniqueId(), connection);
connection.disconnected(); // Only call when actually removed from the map(s).
}
However, this may result in unwanted behavior due to VelocityBossBarImplementation storing a Set<ConnectedPlayer> viewers. If a player is added to this list while they haven't been properly registered in VelocityServer yet, this will cause a memory leak. It could be worth it to make sure we only add viewers to bossbars when they've been properly registered (I don't think that's the case here though, because that would mean bossbar packets are being sent before the ServerLoginSuccessPacket, but this is untested).
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by inspecting VelocityServer#unregisterConnection and ConnectedPlayer#disconnected, then reproduce the duplicate-account login described in the issue. Trace registration and boss-bar viewer handling to determine when the disconnect callback is valid; done means failed duplicate connections do not trigger it while genuine unregisters still do, without leaving retained viewers.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100