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 #6598: prevent client crashes due to failing connects #9163

Merged
merged 4 commits into from May 1, 2021

Conversation

rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented May 1, 2021

Closes #6598

Motivation / Problem

See #6598. Though the premise is, if you are in a network game and you use (re)connect from the console you can get into a weird limbo state when that attempt fails. This is mostly due to NetworkDisconnect closing/freeing things that might be used later, causing many potential places with reading invalid data.

Description

The first step is to remove the NetworkDisconnect from the connect command. This solves all cases where a client tries to join an invalid company number.
The second step is more involved. Upon the join it checks whether the user is in main menu. When it is in the main menu, it will happily continue. When it is not in the main menu, it will load the main menu and trigger the join process to start.

Limitations

Reconnect and connect from within a network game is now slower as it first needs to load the main menu.

Backporting

Backport the following commits:

Leave the following two commits out / do not backport:

  • Change: [Console] Show help when passing invalid company number -> mostly required due to another change that is not in 1.11, which moved the validation out of the console command
  • Codechange: Move join information into a single structure -> makes the code nicer/more maintainable, but not needed for the fix

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')

@rubidium42 rubidium42 added the backport requested This PR should be backport to current release (RC / stable) label May 1, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

This PR feels a bit messy till I started to look at the commits one by one. There are two seemly unrelated fixes in this PR.

I would suggest we do not backport the first part, as it touches parts that should not go in 1.11 to start with.

The second part might be worth it, but honestly, I don't know if it does. If we do, it might be good to split this into two PRs, but I leave this fully up to you :)

src/network/network_client.h Outdated Show resolved Hide resolved
src/openttd.cpp Outdated Show resolved Hide resolved
src/network/network_client.h Outdated Show resolved Hide resolved
src/network/network.cpp Show resolved Hide resolved
src/network/network.cpp Outdated Show resolved Hide resolved
src/network/network.cpp Outdated Show resolved Hide resolved
…oin from within a network game

One could join a network game from within an already running network game. This would call a NetworkDisconnect, but keeps the UI alive. If, during that process the join is aborted, e.g. by cancelling on a password dialog, you would still be in your network game but also get shown the server list.
Solve all the underlying problems by falling back to the main UI when (re)connecting to a(nother) server.
@rubidium42 rubidium42 merged commit 05394d5 into OpenTTD:master May 1, 2021
@rubidium42 rubidium42 deleted the issue-6598 branch May 1, 2021 16:30
@LordAro LordAro added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible for client to be in a nonexistent company on a server; but then the client crashes.
3 participants