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.

Sign up for GitHub

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

Jump to bottom

ThreadSanitizer reports a data race on startup #10

Open
TedLyngmo opened this issue Apr 10, 2023 · 8 comments
Open

ThreadSanitizer reports a data race on startup #10

TedLyngmo opened this issue Apr 10, 2023 · 8 comments
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@TedLyngmo
Copy link
Collaborator 

WARNING: ThreadSanitizer: data race (pid=3539031)
  Write of size 8 at 0x7b2800000838 by main thread (mutexes: write M83):
    #0 recvmsg <null> (libtsan.so.2+0x5945e)
    #1 pa_iochannel_read_with_ancil_data ../src/pulsecore/iochannel.c:445 (libpulsecommon-16.1.so+0x36cab)
    #2 MainWindow::MainWindow() /home/ted/git/The_Great_War/MainWindow.cpp:14 (tsan+0x452b47)
    #3 MainWindow::Instance() /home/ted/git/The_Great_War/MainWindow.cpp:8 (tsan+0x4529c1)
    #4 main /home/ted/git/The_Great_War/MainProg.cpp:5 (tsan+0x452939)

  Previous write of size 8 at 0x7b2800000838 by thread T1:
    #0 recvmsg <null> (libtsan.so.2+0x5945e)
    #1 pa_iochannel_read_with_ancil_data ../src/pulsecore/iochannel.c:445 (libpulsecommon-16.1.so+0x36cab)

  Location is heap block of size 152 at 0x7b2800000820 allocated by main thread:
    #0 malloc <null> (libtsan.so.2+0x3f618)
    #1 pa_xmalloc <null> (libpulse.so.0+0x3749a)
    #2 MainWindow::MainWindow() /home/ted/git/The_Great_War/MainWindow.cpp:14 (tsan+0x452aec)
    #3 MainWindow::Instance() /home/ted/git/The_Great_War/MainWindow.cpp:8 (tsan+0x4529c1)
    #4 main /home/ted/git/The_Great_War/MainProg.cpp:5 (tsan+0x452939)

  Mutex M83 (0x7b0c00002f70) created at:
    #0 pthread_mutex_init <null> (libtsan.so.2+0x50ec8)
    #1 SDL_CreateMutex_REAL /usr/src/debug/SDL2-2.26.3-1.fc37.x86_64/src/thread/pthread/SDL_sysmutex.c:59 (libSDL2-2.0.so.0+0x1407fe)
    #2 MainWindow::MainWindow() /home/ted/git/The_Great_War/MainWindow.cpp:14 (tsan+0x452aec)
    #3 MainWindow::Instance() /home/ted/git/The_Great_War/MainWindow.cpp:8 (tsan+0x4529c1)
    #4 main /home/ted/git/The_Great_War/MainProg.cpp:5 (tsan+0x452939)

  Thread T1 'PulseHotplug' (tid=3539034, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.2+0x5f0e6)
    #1 SDL_SYS_CreateThread /usr/src/debug/SDL2-2.26.3-1.fc37.x86_64/src/thread/pthread/SDL_systhread.c:118 (libSDL2-2.0.so.0+0x9363b)
    #2 SDL_CreateThreadWithStackSize_REAL /usr/src/debug/SDL2-2.26.3-1.fc37.x86_64/src/thread/SDL_thread.c:373 (libSDL2-2.0.so.0+0x9363b)
    #3 MainWindow::MainWindow() /home/ted/git/The_Great_War/MainWindow.cpp:14 (tsan+0x452aec)
    #4 MainWindow::Instance() /home/ted/git/The_Great_War/MainWindow.cpp:8 (tsan+0x4529c1)
    #5 main /home/ted/git/The_Great_War/MainProg.cpp:5 (tsan+0x452939)

SUMMARY: ThreadSanitizer: data race (/lib64/libtsan.so.2+0x5945e) in recvmsg
@TedLyngmo TedLyngmo changed the title ThreadSanitizer reports a data race ThreadSanitizer reports a data race on startup Apr 10, 2023
@PanosPetras PanosPetras added the bug Something isn't working label Apr 11, 2023
@PanosPetras
Copy link
Owner

From the trace it seems like it is something SDL related. It makes sense because we don't use the thread library in the MainWindow class. I believe that this is related to the context classes.

@TedLyngmo
Copy link
Collaborator Author

Yeah, in MainWindow.cpp:14 the SDL_Renderer_ctx is initialized - but it's initialized in the correct order so the pulseaudio initialization (which I guess is done by SDL_Init(SDL_INIT_VIDEO | SDL_INIT_AUDIO) should already be done just like it was prior to the added context classes. Oh well, I can't test this with sound. Does the sound still work? :-)

@PanosPetras
Copy link
Owner

I think the sound still works fine in the game. I will test it again when I have the chance and will inform you.

@PanosPetras
Copy link
Owner

Confirmed: Sound works.

@PanosPetras
Copy link
Owner

Is this issue still present at the current commit?

@TedLyngmo
Copy link
Collaborator Author

TedLyngmo commented Apr 18, 2023 •
edited
Loading

Is this issue still present at the current commit?

Yes, this issue is actually a race condition inside the SDL library - or perhaps in the PulseAudio library that it uses:
libsdl-org/SDL#7427

It was just recently reported to the SDL maintainers but they've already set a target milestone on it and hope to fix it in SDL v2.28.0. I'll keep this ticket open so that I don't forget about it in the future and open a new ticket just like this one :-)

@PanosPetras
Copy link
Owner

So we can't do anything about this. Bottom line is that we upgrade to SDL 2.28.0 once it becomes available. Should we close this issue or should we keep it open in order to remember to keep an eye out for when it gets fixed?

@TedLyngmo
Copy link
Collaborator Author

I'd prefer to keep it open because my memory is so bad that I'll likely open a new ticket about this in a week if I don't see this one :-D

TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 18, 2023
@TedLyngmo
Fix race conditions
1656731 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've removed the asset and map loading threads to get rid of those
races.

I've replaced the remaining date-ticker-thread with a `std::jthread`
which can be used to start a thread in a member function (unlike
SDL_CreateThread).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
@TedLyngmo TedLyngmo mentioned this issue Apr 18, 2023
Closed
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 18, 2023
@TedLyngmo
Fix race conditions
0fbea52 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 18, 2023
@TedLyngmo
Fix race conditions
1b7e620 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've removed the asset and map loading threads to get rid of those
races.

I've replaced the remaining date-ticker-thread with a `std::jthread`
which can be used to start a thread in a member function (unlike
SDL_CreateThread).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 18, 2023
@TedLyngmo
Fix race conditions
e8b6710 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 18, 2023
@TedLyngmo
Fix race conditions
8e26ccf 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 18, 2023
@TedLyngmo
Fix race conditions
1fc6f6c 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 18, 2023
@TedLyngmo
Fix race conditions
4b8664e 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 19, 2023
@TedLyngmo
Fix race conditions
402ed5a 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 19, 2023
@TedLyngmo
Fix race conditions
4f18705 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 19, 2023
@TedLyngmo
Fix race conditions
539d309 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 19, 2023
@TedLyngmo
Fix race conditions
3500858 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 19, 2023
@TedLyngmo
Fix race conditions
aff853b 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 19, 2023
@TedLyngmo
Fix race conditions
610708a 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo added a commit to TedLyngmo/The_Great_War that referenced this issue Apr 19, 2023
@TedLyngmo
Fix race conditions
252b953 
There are currently three extra threads started by the application. None
do synchronization between the threads so there are race conditions.

I've replaced the threads with `std::jthread`s which can be used to
start a thread in a member function (unlike `SDL_CreateThread`).

`bIsPaused` is made into a `std::atomic<bool>` and moved out of the
`Date` struct.

No further synchronization work has been done - but it's needed.

One positive effect is that the memory leaks are now very small due to
the auto-joining feature of the `std::jthread`. For me, 200 bytes are
leaked which is likely unavoidable due to SDL internals.
Perhaps PanosPetras#1 can be
closed.

Note: This has no impact on PanosPetras#10

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
@PanosPetras PanosPetras added the wontfix This will not be worked on label Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@TedLyngmo @PanosPetras