Fix #9501: [Network] crash when more than one game-info query was pending #9502
Conversation
TrueBrain
force-pushed
the
fix-multiple-servers
branch
from
793839d
to
bd21ae7
Compare
| #include "network_internal.h" | ||
|
|
||
| /** Class for handling the client side of quering a game server. */ | ||
| class QueryNetworkGameSocketHandler : public ZeroedMemoryAllocator, public NetworkGameSocketHandler { |
There was a problem hiding this comment.
Is the ZeroedMemoryAllocator really needed here? Only the ClientNetworkGameSocketHandler uses it but not the ServerNetworkGameSocketHandler so it is not something the NetworkGameSocketHandler intrinsicly needs. Since this has only one instance variable that gets in the constructor, I think there is no use for it.
There was a problem hiding this comment.
ServerNetworkGameSocketHandler also zero's the object, but this is done by the pool (Tzero is set to true). So there too Calloc is used. In other words: it currently is consistent in zero'ing the object on creation.
Of course these days we can just set the initial values of variables, and possibly get ride of this whole ZeroedMemoryAllocator, but that might be best for another PR. For now, I suggest we leave it consistent, and zero the object for all children using NetworkGameSocketHandler.
TrueBrain
changed the title
Motivation / Problem
When you have more than one query-for-game-info pending, the game crashes. This is easiest reproduced by starting a local server, and adding
127.0.0.1and127.0.0.2to your server-list. Close the game, restart it, click Multiplayer. Now two queries will be send out at the exact same time.Description
We piggy-backed on the client code to, after establishing a TCP connection, query the server. The client code only has a very weird and annoying to follow singleton called
my_client. As it turns out, when it is being asked to query two different servers at the same time, it panics.To resolve the situation in a more robust way, I cut out the game-info query part from the client code, and moved it to
network_query. This part is now responsible solely for querying the game-info of a server, and can have as many parallel connection as needed. It self-registers and self-destroys.As an additional bonus, this means we can also handle errors from servers in a bit nicer way for when a query fails. This is best seen about the code removed from the client code.
As the protocol reuses other packages to indicate issues, the server can send back 4 different packets depending on the situation on a game-info request:
game-info, what we are after.error, when something weird went wrong. For example, when it never saw your game-info request packet .. it will hit a timeout and close your connection. But this is really unlikely to ever happen.banned, when your IP is banned (you won't be able to query the server in that case either; only seems fair).full, which is already a bug on our bugtracker, but currently at a very early moment in time we validate if the server has room for you. If not, you get this packet back, and refresh fails. This of course has to be resolved, but as for now, this is still happening.Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.