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 #9388: thread unsafe use of NetworkAdminConsole/IConsolePrint #9456

Merged
merged 1 commit into from Sep 1, 2021

Conversation

rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Jul 23, 2021

Motivation / Problem

Fixes #9388.
Some debug messages are sent from other threads than the GameLoop. With NetworkAdminConsole this might cause multiple threads adding to the queue at the same time corrupting the (linked) list of Packets. With IConsolePrint this could also corrupt the (linked) list of lines in the console.

Description

Queue the NetworkAdminConsole/IConsolePrint calls from Debug (when needed) and actually "send" those from the GameLoop.

Limitations

None that I know about.

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

JGRennison pushed a commit to JGRennison/OpenTTD-patches that referenced this pull request Jul 26, 2021
(cherry picked from commit d7a0e80fc18b689455eaca09883fccbcc722d7e3)

# Conflicts:
#	src/debug.cpp
#	src/debug.h
#	src/table/settings/gui_settings.ini

See: OpenTTD/OpenTTD#9456
@TrueBrain TrueBrain added this to the 1.12 milestone Aug 14, 2021
@Xaroth
Copy link
Contributor

Xaroth commented Aug 31, 2021

As mentioned on IRC, I added an example to libottdadmin2 that will listen to console data and output it, hopefully this can help you test this PR. The output is quite spammy, so wear earmuffs or something against the noise.

Assuming you have python3.5+ installed with venv:

To install a local env:

git clone https://github.com/Xaroth/libottdadmin2.git && cd libottdadmin2
python3 -m venv .venv
.venv/bin/pip install -e .

To run:

.venv/bin/python example/syncio_log_client.py <args>  # << --help shows all command line options

Feel free to poke me on IRC if anything else is needed.

@rubidium42 rubidium42 marked this pull request as ready for review September 1, 2021 20:13
@rubidium42
Copy link
Contributor Author

Did some testing with Xaroth's library. I did not get any warnings from the ThreadSanitizer with debug messages getting sent to the admin clients, even when these debug messages are created in a separate thread.

@rubidium42 rubidium42 merged commit 92559e6 into OpenTTD:master Sep 1, 2021
@rubidium42 rubidium42 deleted the pr9388 branch September 1, 2021 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin port: No synchronisation in NetworkAdminConsole which may be called from any thread
3 participants