Fix inconsistency with player logins vs player disconnects

[f4n74z14]

Current issues:

1. Not in all cases plugins can track player disconnection immediately after LoginEvent.
2. Several LoginEvent are possible for one player. (A player may connect while other plugins are processing LoginEvent)

This PR solves these problems:

1. Added LoginAbortEvent, which is called when LoginEvent is completed, before PostLoginEvent and the player has left the server.
2. Added synchronization of logins by nickname. If a player logged in twice, only the first one will be allowed.

[Janmm14]

Until now, PlayerDisconnectEvent has only been used when the client <-> bungee connection has been terminated by the client.

The disconnect event call in line 581 however is a forced kick from bungee.

We either need a new event or add an reason/cause enum field.
I think some plugins might want to handle both cases identical tho, so I would just add a reason enum.

There are also a bunch of other cases where InitialHandler disconnects a player, why you chose just these cases?

[f4n74z14]

Hi! PlayerDisconnectEvent is called also if connection was terminated by the server.

I can implement your idea, do you have any suggestions for improving an existing event or creating a new one?

I chose this case because in this case LoginEvent is called, and logically there should be a PlayerDisconnectEvent after that, otherwise the state of some plugins might not be correct. PreLoginEvent is too early for this, but most plugins that load the cache work specifically with LoginEvent.

[Janmm14]

Hi! PlayerDisconnectEvent is called also if connection was terminated by the server.

My bad, I was just seeing it being called in UpstreamBridge before, but UpstreamBridge's disconnected (by nettys channelInactive) will also be called when we close the connection.
Due to this, the PR obviously does not need any kind of additonal indication whether the connection close was initiated by the client or the server.

Seeing how this is handled in UpstreamBridge, we should move the event call also into the disconnected function here, but only if the state is FINISHING.

[bob7l]

The only problem I see here is plugins atm expect the PlayerDisconnectEvent to ONLY be called AFTER the PostLoginEvent - so they usually populate maps/data pertaining to the UserConnection on the PostLoginEvent event instead.

This commit changes the flow, and will fire a PlayerDisconnectEvent during, after, and if the result of the PlayerDisconnectEvent is cancelled.

Could lead to unintended plugin behavior, or nullpointers/errors from plugins. Your plugin could still be in the middle of an asynchronous process holding a LoginEvent intent - and get a PlayerDisconnectEvent before you finish.

Unless I'm misunderstanding of course.

[Janmm14]

The only problem I see here is plugins atm expect the PlayerDisconnectEvent to ONLY be called AFTER the PostLoginEvent - so they usually populate maps/data pertaining to the UserConnection on the PostLoginEvent event instead.

This commit changes the flow, and will fire a PlayerDisconnectEvent during, after, and if the result of the PlayerDisconnectEvent is cancelled.

Could lead to unintended plugin behavior, or nullpointers/errors from plugins. Your plugin could still be in the middle of an asynchronous process holding a LoginEvent intent - and get a PlayerDisconnectEvent before you finish.

Unless I'm misunderstanding of course.

Yeah this could be possible, I didn't think about asnyc events :(
Then its not so good to move the event calling to the disconnected function :/

[f4n74z14]

Your plugin could still be in the middle of an asynchronous process holding a LoginEvent intent - and get a PlayerDisconnectEvent before you finish.

Dsconnected will not be called until the intent is completed, or until the player rejoins during the registered intent.
The main problem is that because it is possible to call multiple LoginEvent (If the same player connects multiple times per second), there will be multiple PlayerDisconnectEvent (possible even after player rejoins).
At the moment there will be 0 PlayerDisconnectEvent per several LoginEvent.

The only problem I see here is plugins atm expect the PlayerDisconnectEvent to ONLY be called AFTER the PostLoginEvent

Depends to plugin. Example

[bob7l]

Dsconnected will not be called until the intent is completed, or until the player rejoins during the registered intent.

Not true with your current implementation. InitialHandler is a PacketHandler, so disconnected(ChannelWrapper) will be called if the channelInactive event is ever fired which isn't bound to this event. Therefor if a Plugin executes an asynchronous task during the event, and registers it's intent, the PlayerDisconnectEvent has the potential to be called before the event is done.

Depends to plugin. Example

It wont be a problem with most plugins I'm sure. But having tried something similar to this in the past, some plugins will throw exceptions due to not expecting a PlayerDisconnectEvent to be possible before a PostLoginEvent.

[f4n74z14]

Not true

It's true. I discovered this experientially, registered an intent and then logged out of the server. PlayerDisconnectEvent was called after the old connection was timedout / was terminated by the client.

It wont be a problem with most plugins I'm sure. But having tried something similar to this in the past, some plugins will throw exceptions due to not expecting a PlayerDisconnectEvent to be possible before a PostLoginEvent.

That doesn't mean the change is bad, it's just a matter of deciding what's more important.

The important problem in this solution is that PlayerDisconnectEvent can be called even if the player is online, in case the player has logged into the server multiple times at the same time. (After testing, it turns out that Velocity behaves the same way)

[bob7l]

It's true. I discovered this experientially, registered an intent and then logged out of the server. PlayerDisconnectEvent was called after the old connection was timedout / was terminated by the client.

Maybe we're misunderstanding each-other here. So I'll demonstrate what I mean by flow:

15:21:38 [INFO] [/127.0.0.1:25484] <-> InitialHandler has pinged
15:21:42 [INFO] [/127.0.0.1:25510] <-> InitialHandler has connected
15:21:42 [INFO] [TestPlugin] LoginEvent start, player connected: true
15:21:43 [INFO] [TestPlugin] PlayerDisconnectEvent called
15:21:47 [INFO] [TestPlugin] LoginEvent end, player connected: false

See what I mean? With your commit, the PlayerDisconnectEvent can happen during the LoginEvent.
Example code: https://pastebin.com/mdns21BG

That doesn't mean the change is bad, it's just a matter of deciding what's more important.

True, it's more of a "Should we remain compatible with older plugins, or improve the API". Like what was suggested here by Jamm, maybe it would be more suitable to introduce a new event rather than disrupting the flow of an existing event.

[f4n74z14]

Maybe we're misunderstanding each-other here.

Yeah, I didn't think of that one.

True, it's more of a "Should we remain compatible with older plugins, or improve the API". Like what was suggested here by Jamm, maybe it would be more suitable to introduce a new event rather than disrupting the flow of an existing event.

At the beginning I didn't realize that's what you meant.
I think it's right to split these events into different ones to maintain compatibility, but also wanted to hear md_5's opinion.

[md-5]

I haven't had a chance to look closely but I think this event may also be problematic in terms of using an uninitialised UserConnection; and also should plugins really be maintaining player state prior to the player actually logging in

[f4n74z14]

I haven't had a chance to look closely but I think this event may also be problematic in terms of using an uninitialised UserConnection; and also should plugins really be maintaining player state prior to the player actually logging in

As we said earlier, we can think of another event, for example LoginAbortedEvent, where no UserConnection will be used, but only PendingConnection.

This would be an auxiliary event to allow plugins to track when a player dropped the connection too early.

I also have a test implementation that solves the problem of multiple LoginEvent calls for a single player, it works on an extra list for InitialConnection, which in theory could slow down bandwidth, but I don't know how to do it any other way.

[bob7l]

As we said earlier, we can think of another event, for example LoginAbortedEvent, where no UserConnection will be used, but only PendingConnection.

This would be an auxiliary event to allow plugins to track when a player dropped the connection too early.

I also have a test implementation that solves the problem of multiple LoginEvent calls for a single player, it works on an extra list for InitialConnection, which in theory could slow down bandwidth, but I don't know how to do it any other way.

That's actually a great name for it.

LoginEvent/PreLoginEvent are the only asynchronous events for properly loading data and what not. So there's for sure a deep need for some sort of disconnection event.

[f4n74z14]

PR has been updated to take into account the comments above: it is now impossible to call the case when the player is online and PlayerDisconnectEvent or LoginAbortEvent is called.
Added LoginAbortEvent.

[Outfluencer]

If you want to check if the player has been disconnected during the login, can't you just listen on the event with highest event prio and check if connection has been closed?

You could check that the Login Event is canceled, and you could check if the connection is closed In post Login at highest event prio or something

[f4n74z14]

If you want to check if the player has been disconnected during the login, can't you just listen on the event with highest event prio and check if connection has been closed?

You can't check it here .

[Outfluencer]

Is this even possible to be closed in this specific code area.
Maybe this check can be removed, but i am not sure cause we execute in netty eventloop

we could change the first isClosed to isClosing and remove the second isclosing check

[Outfluencer]

I also think adding a lock in intial handler is not very smart, if much connections are spamming and for every disconnect and login an object locks on all netty threads happens, it can cause the threads to wait and maybe the legit connections are lagging

[f4n74z14]

we could change the first isClosed to isClosing and remove the second isclosing check

I don't think so, because during the context change the thread is unblocked and closing the connection is possible during this interval.

I also think adding a lock in intial handler is not very smart, if much connections are spamming and for every disconnect and login an object locks on all netty threads happens, it can cause the threads to wait and maybe the legit connections are lagging

it works on an extra list for InitialConnection, which in theory could slow down bandwidth, but I don't know how to do it any other way

This is a known problem, if you have a suggestion to solve this problem, please advise.

[Outfluencer]

You could add an connection closed event for all Connections and than let the plugin devs do whatever they want to with it, they could check if the conenction is instance of PendingConnection and so on, but i am not sure if this is what you want

[Outfluencer]

I only think that this is not a valid solution because of the thread locks

[Outfluencer]

Btw, an evil person could prevent your players from joining if he creats connections with their usernames and send the login packets, if he had a list of the players he could constanly let them join with no authentification. and the real players would get disconnected all the time because "their already connected"