Login performance #1331
Login performance #1331
Conversation
|
@ljacqu what do you think about this? ;) |
|
hi @sgdc3, this looks cool! This will make the verifications happen in async for server implementations where it's possible. Hopefully this is helpful for us. One thing I'm unsure about is if Edit: Actually I think it's quite a difference given that if we don't hook into a specific perms system, the default permission (true / op / false) will be considered. IMHO a breaking change I'm not sure is worth to introduce? |
| onJoinVerifier.checkKickNonRegistered(isAuthAvailable); | ||
| onJoinVerifier.checkAntibot(name, isAuthAvailable); | ||
| onJoinVerifier.checkNameCasing(name, auth); | ||
| onJoinVerifier.checkPlayerCountry(name, ip, isAuthAvailable); |
There was a problem hiding this comment.
One iteration beyond this (but only once we resolve this permission behavior change!) is to look at whether we really need the auth object at all? As far as I see here from a quick look, we only want to know whether isAuthAvailable and then I only see it used in checkNameCasing(name, auth).
If instead we can have a dedicated method to return the real name (similar to DataSourceResult<String> getEmail(String)) we avoid constructing PlayerAuth objects, which is more costly.
| // Note #831: AsyncPlayerPreLoginEvent is not fired by all servers in offline mode | ||
| // e.g. CraftBukkit does not fire it. So we need to run crucial things with PlayerLoginEvent. | ||
| // Single session feature can be implemented for Spigot and CraftBukkit by canceling a kick | ||
| // event caused by "logged in from another location". The nicer way, but only for Spigot, would be | ||
| // to check in the AsyncPlayerPreLoginEvent. To support all servers, we use the less nice way. | ||
|
|
||
| private boolean isAsyncPlayerPreLoginEventCalled = false; |
There was a problem hiding this comment.
Fields go to the top of the class :) Thanks
There was a problem hiding this comment.
Forgot to move that, sorry xD
|
|
||
| if (validationService.isUnrestricted(name)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Here, like in the non-async method, would it be worth to check event#result? I don't think it's dramatic as it is right now, but we'd be more consistent with the same check.
There was a problem hiding this comment.
I think it would break some features like the single session kick message.
| teleportationService.teleportOnJoin(player); | ||
|
|
There was a problem hiding this comment.
This introduces a change in behavior I'm not sure is intentional—previously with the red block we did all of the onJoin verifications before and kicked the player. With the green addition the player gets teleported before any checks are made.
This is maybe better because this way a player is teleported as fast as possible (since we don't want to show the original location before login in many cases) ? But at the same time in the case of a bot attack we might be teleporting a lot of players before determining that they have no right being online. Difficult...
There was a problem hiding this comment.
Teleporting players to a same spot shouldn't be resource intensive
Could we chat about that on discord? @ljacqu |
sgdc3
force-pushed
the
login-performance
branch
from
896ad23
to
50f094f
Compare
|
Ready to merge? |
|
I'll test this locally and merge afterwards. |
|
Single session worked fine for me on Spigot 1.11.2. Didn't test on Bukkit but the mechanism shouldn't change at all, so now I merge. |
No description provided.