RCONNoAuthException when trying to use changelevel

[AnAkkk]

Apparently there is a bug in steam-condenser-java when sending the changelevel command on a source server through rcon.
The rcon auth works fine, but it will always return an exception when sending the command:

com.github.koraktor.steamcondenser.exceptions.RCONNoAuthException: Not authenticated yet.

From my understanding and debugging, what happens is:

1. The auth packet is sent to the server
2. The server replies back that the auth was successfull
3. The changelevel command is sent to the server
4. The server replies back with the response, it worked fine, and it start changing level
5. steam-condenser send another packet in rconExec for the "Multi-packet response" workaround, as described on https://developer.valvesoftware.com/wiki/Source_RCON_Protocol#Multiple-packet_Responses
6. That's where it fails, the server doesn't reply because it's changing level:
   this.rconSocket.getReply() calls receivePacket(4) which calls read(this.buffer). read returns -1, which makes receivePacket(4) return 0, and getReply return NULL.
   It enters the condition:

if (responsePacket == null ||
responsePacket instanceof RCONAuthResponse) {
this.rconAuthenticated = false;
throw new RCONNoAuthException();
}

And then return an exception, although the changelevel worked and it did get the response correctly.

That can most likely be fixed by modifying the condition to:

if (responsePacket == null &&
responsePacket instanceof RCONAuthResponse)

OR

if ((response.size < 2 && responsePacket == null) ||
responsePacket instanceof RCONAuthResponse)

Although it will still need to exit the loop with a break somewhere since responsePacket is null, otherwise it'll crash.

[koraktor]

Your first change would effectively make this check obsolete, because the if statement can never be true.

The second one seems to be more appropriate, although I’m not entirely sure about all implications of that change.

[AnAkkk]

Have you ever seen a case where responsePacket was null though? Maybe that should just be removed from the condition and exit the loop when it's the case.

[koraktor]

There’s at least the one case where restarting a server leads to the same effect as changelevel. In that case RCON authentication is really invalidated, though.

There where quite a few issue reports about the various edge cases of restarting a server or changing a map. I’ll have to check them again to see if there’s still potential to improve the logic.

[AnAkkk]

The "Multi-packet response" workaround doesn't seem to be needed, at least HLSW doesn't use it from what I can see (tried with just "status"), and it's still able to print "cvarlist" correctly.
EDIT: Actually "cvarlist" was 4050 bytes.
EDIT 2: Even doing "cvarlist;cvarlist" works and HLSW doesn't use the workaround

[koraktor]

To be honest, I never revisited the decision to implement a workaround once it worked for most use cases. It should be possible with TCP streams, but I think there have been problems in the past with that.

[AnAkkk]

Sometimes it also throws a "Connection reset by peer" exception, which is supposed to be "ConnectionResetException" according to your code, but it's really a "SteamCondenserException" because you check against the message string, which doesn't work in my language as the error message is in French.
Anyway it should probably catch that exception inside rconExec if there has already been a response.
EDIT: Actually you do catch that exception in getReply(), but that doesn't work since the wrong exception is thrown for me :/

[koraktor]

@anakin1 About HLSW: Did you mean that the result of calling cvarlist inside HLSW was 4050 bytes long? That’s not complete, the full output of a TF2 server is 203389 bytes long.

[AnAkkk]

Yes, that might have been wrong, although it did receive the full cvarlist without the workaround.

[koraktor]

I just had a look at the available options here.

The original implementation just waits for a timeout. That works, but might lead to incorrect results on unstable connections (or low timeout settings). Additionally it has to wait for the timeout to every RCON request, even on single packet replies.

Other variants like using non-blocking sockets, peeking into the stream and other variants did not really improve the situation here.

The current implementation is stable and fast. Downsides are added packet overhead and more or less undefined behavior during server restarts, map changes etc.

At the moment, I’ll think about sticking with the current one. Your suggested patch (i.e. checking for already read packets) seems valid, but may introduce other edge case bugs.