Skip to content
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

Fix #9501: [Network] crash when more than one game-info query was pending #9502

Merged
merged 1 commit into from Aug 23, 2021

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Aug 21, 2021

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.1 and 127.0.0.2 to 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.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
src/network/network_query.cpp Outdated Show resolved Hide resolved
Loading
src/network/network_query.cpp Outdated Show resolved Hide resolved
Loading
@TrueBrain TrueBrain force-pushed the fix-multiple-servers branch from 793839d to bd21ae7 Aug 21, 2021
#include "network_internal.h"

/** Class for handling the client side of quering a game server. */
class QueryNetworkGameSocketHandler : public ZeroedMemoryAllocator, public NetworkGameSocketHandler {
Copy link
Contributor

@rubidium42 rubidium42 Aug 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@TrueBrain TrueBrain Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@TrueBrain TrueBrain changed the title Fix #9501: [Network] crash when more than one game-info query was pen… Fix #9501: [Network] crash when more than one game-info query was pending Aug 23, 2021
@TrueBrain TrueBrain merged commit b2f0491 into OpenTTD:master Aug 23, 2021
15 checks passed
Loading
@TrueBrain TrueBrain deleted the fix-multiple-servers branch Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants