PaperMC / PaperMC/Velocity

DisconnectEvent covers too many cases, leading to surprising behavior

Open
#1,691 0 comments 3 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

type: feature
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 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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.