DisconnectEvent covers too many cases, leading to surprising behavior
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 2.3k
- Forks
- 959
- Avg merge
- 5d 10h
- Merged PRs (30d)
- 2
Description
Requested Feature
I'd like to see the behavior of DisconnectEvent slightly changed for a more user-friendly (and less surprising) API.
Currently, DisconnectEvent is called when any player connection is closed for any reason at any point in the connection's lifecycle, this includes disconnects triggered by the user early in the login process, disconnects caused by the player already being on the server (with the kick option turned off), and disconnects caused by the pre-login events being cancelled. In all three of these cases, I think it's pointless to fire a DisconnectEvent when the corresponding LoginEvent has not been fired.
Furthermore, it's not immediately apparent (and not documented at all, actually) that DisconnectEvent covers so many cases, which means code like this is very common:
@Subscribe
public void onLogin(LoginEvent event) {
// e.g. create player data and add it to a UUID -> Data map
initialize(event.getPlayer().getUniqueId());
}
@Subscribe
public void onDisconnect(DisconnectEvent event) {
// e.g. remove the UUID from the map
cleanup(event.getPlayer().getUniqueId());
}
Some real-world examples: LuckPerms, BungeeTabListPlus, BuycraftX -- all these seemingly break when DisconnectEvent gets fired unexpectedly.
This code has a problem that is not obvious at all, when e.g. the player attempts to log in while already connected (and with the kick option turned off), DisconnectEvent is fired for another connection but with the same UUID, meaning cleanup gets run but the player remains online.
What I suggest is either:
- Only fire
DisconnectEventwhen the correspondingLoginEventhas already been fired (i.e. for statusesSUCCESSFUL_LOGIN,CANCELLED_BY_USER_BEFORE_COMPLETE, andPRE_SERVER_JOIN). For the other statuses, introduce another "lower-level" event (e.g.ConnectionCloseEventor similar) - Alternatively to the above, introduce a higher-level event (e.g.
LogoutEvent), that is fired for connections that have fired aLoginEvent. - Introduce an API to check whether the login status has fired its
LoginEvent, right now you have to dig into Velocity's source to understand which statuses are "safe" to cleanup from, and this may change on proxy updates. - Document this quirk and suggest e.g. checking the status, or checking that
event.getPlayer()is the expectedPlayerthat is currently connected.
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 tracing the DisconnectEvent and LoginEvent entry points in the Velocity source, including the listed login statuses, to map when each event is dispatched. No specific files or tests are named in the issue. Done requires a decided event-lifecycle design, corresponding behavior or API changes, and documentation covering early and unsuccessful connection cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend-api-design
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100