-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add PlayerConfigurationEvent #3877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Closes #3869 |
proxy/src/main/java/net/md_5/bungee/connection/DownstreamBridge.java
Outdated
Show resolved
Hide resolved
…s not in login state
…ckets to the player in the players eventloop
api/src/main/java/net/md_5/bungee/api/event/PlayerConfigurationEvent.java
Outdated
Show resolved
Hide resolved
proxy/src/main/java/net/md_5/bungee/connection/UpstreamBridge.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Janmm14 <[email protected]>
|
I have tested this heavily now on 1.21.8, joining, switching servers & backend reconfigiring works all perfectly fine. Also slightly tested the other versions, and encounter no problems |
|
@md-5 what do you think about this impl? |
|
Let's look after 1.21.9. I'm worried about making the configuration phase even more complicated |
|
time to take a look @md-5? |
| if ( packet.packet == null && packet.protocol == Protocol.CONFIGURATION && server.isQueuingConfigPackets() ) | ||
| { | ||
| // I want to be 100% sure this does not leak, so we use an unpooled copy. | ||
| ByteBuf unpooled = Unpooled.copiedBuffer( packet.buf ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just .retain the original?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but we have to release them if the connection unexpectedly closes then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An unpooled buffer can leak too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought unpooled heap buffers are always just garbage collected without any problems, anyways i habe changed it to retain the buffer and release them in the channels close future
|
|
||
| receivedLogin = true; | ||
| ServerConnector.handleLogin( bungee, server.getCh(), con, server.getInfo(), null, server, login ); | ||
| // if the backend reconfigures the connection it can send multiple login packets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we handle those multiple packets then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd say no, if we would calll ServerConnector.handleLogin again it would break api and call ServerConnected Event multiple times for the server just because its reconfiguring. also it would clear scoreboards and tablist then, even if the client has not really switched the server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also as fas as i know vanilla/datapacks do not reconfigure the client and spigot has no api for it, to reconfigure the client you would have to use nms or paper api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we disconnect then? The server is doing something and we're just ignoring it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why, the backend reconfiguring the client shouldn’t break anything. We can just let it happen.
I am not sure what specificly should we handle in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backend is sending a packet and we're just blocking it from doing anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we blocking it if its not the first packet it does not throw CancelSendSignal
the packet just goes through or not?
…sconnected. this will leave the client in a half configured state
I have tested this on latest, also tested switching servers.
Tested with this sample plugin