Forward DisconnectPacket to client in ConfigSessionHandler and TransitionSessionHandler

[R00tB33rMan]

This resolves #1603 by forwarding the DisconnectPacket correctly during both phases and was inspired by the precise code mentioned in the issue report.

[electronicboy]

This is changing from closing the backend connection to closing the players connection directly, this to my understanding should ideally be handled through the future, that way the caller can do what it needs to do, i.e. fitting in standard message templating

[HP888]

This is changing from closing the backend connection to closing the players connection directly, this to my understanding should ideally be handled through the future, that way the caller can do what it needs to do, i.e. fitting in standard message templating

I understand that it was designed in such a way, but do Velocity expose API to get out of the CONFIGURATION state (Reconfiguring... GUI)? If don't then I don't think that it's a good idea to keep it this way, because what else could we do besides disconnecting the player?

[electronicboy]

I'm not saying that the disconnection of the player is wrong, I'm saying that we probably should keep the server connection closed and deal with the disconnect in the future

[HP888]

Oh okay, that makes sense.

[electronicboy]

I accidently reformatted too much in my IDE to just generate a patch, but, I was pondering over something like

@@ -1438,8 +1445,15 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player,
           VelocityServerConnection con =
               new VelocityServerConnection(vrs, previousServer, ConnectedPlayer.this, server);
           connectionInFlight = con;
-          return con.connect().whenCompleteAsync((result, exception) -> this.resetIfInFlightIs(con),
-              connection.eventLoop());
+          return con.connect().whenCompleteAsync((result, exception) -> {
+            if (result != null && !result.isSuccessful() && !result.isSafe()) {
+              result.getReasonComponent()
+                  .ifPresent(reason -> handleConnectionException(result.getAttemptedConnection(),
+                      DisconnectPacket.create(reason, getProtocolVersion(), connection.getState()),
+                      false));
+            }
+            this.resetIfInFlightIs(con);
+          }, connection.eventLoop());
         }, connection.eventLoop());
       });
     }

This might need other adjustements, but, if the proposed solution works, I think that this is a cleaner setup? The things inside of the ConfigSessionHandler also needs its method adjusting to unsafe, and likely add a variant to be able to pass in the disconnection component

[R00tB33rMan]

I accidently reformatted too much in my IDE to just generate a patch, but, I was pondering over something like

@@ -1438,8 +1445,15 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player,
           VelocityServerConnection con =
               new VelocityServerConnection(vrs, previousServer, ConnectedPlayer.this, server);
           connectionInFlight = con;
-          return con.connect().whenCompleteAsync((result, exception) -> this.resetIfInFlightIs(con),
-              connection.eventLoop());
+          return con.connect().whenCompleteAsync((result, exception) -> {
+            if (result != null && !result.isSuccessful() && !result.isSafe()) {
+              result.getReasonComponent()
+                  .ifPresent(reason -> handleConnectionException(result.getAttemptedConnection(),
+                      DisconnectPacket.create(reason, getProtocolVersion(), connection.getState()),
+                      false));
+            }
+            this.resetIfInFlightIs(con);
+          }, connection.eventLoop());
         }, connection.eventLoop());
       });
     }

This might need other adjustements, but, if the proposed solution works, I think that this is a cleaner setup? The things inside of the ConfigSessionHandler also needs its method adjusting to unsafe, and likely add a variant to be able to pass in the disconnection component

This makes a bit more sense to me as modifying these codesections seems problematic in that it diverts from Velocity's codestyle. I'm pretty sure your proposed solution is very much the right solution, so I'll fully rebase this and experiment with it. Thank you!

[R00tB33rMan]

This is changing from closing the backend connection to closing the players connection directly, this to my understanding should ideally be handled through the future, that way the caller can do what it needs to do, i.e. fitting in standard message templating

I understand that it was designed in such a way, but do Velocity expose API to get out of the CONFIGURATION state (Reconfiguring... GUI)? If don't then I don't think that it's a good idea to keep it this way, because what else could we do besides disconnecting the player?

@HP888, feel free to give this latest JAR a spin and let me know if it works correctly on your end as well.

[HP888]

I've had some free time to test it. Unfortunately @R00tB33rMan, the issue still persists on my end - I'm still getting stuck on the "Reconfiguring..." screen.

[R00tB33rMan]

I've had some free time to test it. Unfortunately @R00tB33rMan, the issue still persists on my end - I'm still getting stuck on the "Reconfiguring..." screen.

Thank you for verifying. I’ll look into this more thoroughly then

[R00tB33rMan]

I've had some free time to test it. Unfortunately @R00tB33rMan, the issue still persists on my end - I'm still getting stuck on the "Reconfiguring..." screen.

Feel free to try this again!