DisconnectEvent covers too many cases, leading to surprising behavior

[roccodev]

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 DisconnectEvent when the corresponding LoginEvent has already been fired (i.e. for statuses SUCCESSFUL_LOGIN, CANCELLED_BY_USER_BEFORE_COMPLETE, and PRE_SERVER_JOIN). For the other statuses, introduce another "lower-level" event (e.g. ConnectionCloseEvent or similar)
• Alternatively to the above, introduce a higher-level event (e.g. LogoutEvent), that is fired for connections that have fired a LoginEvent.
• 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 expected Player that is currently connected.