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 bddfcaef: don't tell twice that a client left because of a timeout etc #8746

Merged
merged 1 commit into from Feb 26, 2021

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 25, 2021

Motivation / Problem

While working on something completely unrelated, found that clients that timeout trigger a disconnect message twice.

After that, I got told that "guess everyone thinks that's normal" and "there are worse errors somewhere there".

Well, I disagree! I don't want to be told twice some lame-ass had a bad connection. Get out of here!

:D

PS: this bug is 9 years old.

Description

SendError() notifies all clients of the disconnect. This calls
CloseConnection() at the end, which also notified the clients
of the disconnect. Really no need to do it twice.

The status NETWORK_RECV_STATUS_SERVER_ERROR is only set by
SendError(), so in case that is the status, don't let
ClientConnection() send another notification.

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

SendError() notifies all clients of the disconnect. This calls
CloseConnection() at the end, which also notified the clients
of the disconnect. Really no need to do it twice.

The status NETWORK_RECV_STATUS_SERVER_ERROR is only set by
SendError(), so in case that is the status, don't let
ClientConnection() send another notification.
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Feb 25, 2021

Fun fact / addition: only the server saw this, as the clients got 2 disconnect packets, but once the first was processed, the next becomes invalid, and is silently dropped :P

@TrueBrain TrueBrain merged commit d068d61 into OpenTTD:master Feb 26, 2021
10 checks passed
@TrueBrain TrueBrain deleted the fixing-old-network-bugs branch Feb 26, 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

2 participants