[BUG] TSAN error logs

[rwgk]

TSAN logs as requested by @YannickJadoul here:

#2746 (comment)

Using master b7dfe5c
+ #2409
+ #2746 (YannickJadoul@727b3ae)
+ 2 additional minor local patches.

test -c dbg --config=tsan (Google-internal toolchain)

Failing tests:

• test_gil_scoped.py [EDIT: see PR Add PYBIND11_SIMPLE_GIL_MANAGEMENT option (cmake, C++ define) #4216]
• test_iostream.py [EDIT: see PR Adding iostream.h thread-safety documentation. #2995]:

  pybind11/tests/test_iostream.cpp

  Line 43 in d587a2f

  t_ = new std::thread(std::move(thread_f));

[rwgk]

test_gil_scoped_tsan_log.txt

[rwgk]

test_iostream_tsan_log.txt

[YannickJadoul]

To link up things:

• The test_iostream failure seems to be more info about pythonbuf fix #2675 (comment)

[YannickJadoul]

I knew I'd seen the other one as well:

• The test_gil_scoped race condition is also shown in Race condition in test_python_to_cpp_to_python_from_process test on 3.9? #2391

[bstaletic]

I know this is the TSAN thread, but TSAN seems to be a major PITA to set up. I'm hoping no one would mind me submitting helgrind's analysis.

https://gist.github.com/bstaletic/98441d24b5e0010118051c31036ef2f8

Note: debug symbols for all dependency are necessary to get useful tracebacks.

Observation: test_gil_scoped.py doesn't seem to touch pybind11 as far as threading errors are concerned, but test_iostream.py does.

[YannickJadoul]

The main issue with Helgrind (I've been playing around with it, a few days ago), is that it doesn't recognize C++'s atomic, etc?

Btw, the main issue (or one of the main issues) seems to lie in this line:

pybind11/tests/test_iostream.cpp

Line 40 in e612043

std::cout << "x" << std::flush;

Apparently, std::cout is supposed to be thread-safe though (guaranteed by the standard, thread-safe; not garbled-output safe, but let's not care about that for now) ? However, I don't know if that's still the case for std::cout with the custom streambuf?

[rwgk]

@henryiii did someone already try out this suggestion in the meantime?
https://github.com/pybind/pybind11/blame/master/include/pybind11/iostream.h#L41

[henryiii]

The current implementation is exactly what is recommended. The "fully qualified virtual call" is at best the same, and possibly not really functional in forcing the correct call. Since it didn't fix the problem, I don't think there's any harm in moving to the fully qualified call, but there should not be a benefit.

[henryiii]

Do you think we could add a mutex here, perhaps?

[rwgk]

I really don't know, not knowing much about threading, but I'd be happy to try out ideas for you. If you point me to a branch with experimental changes it'll take me only a few minutes to try them out.

[YannickJadoul]

@henryiii, @rwgk and I discussed this in a private chat, but we couldn't figure out who's job it is to have the mutex: the custom streambuf or the part of the program that's writing to std::cout?

According to https://berthub.eu/articles/posts/iostreams-unexpected/ (quoting the standard), writing to std::cout from different threads can garble up the output, but shouldn't be UB. So that kind of makes me think the synchronization needs to happen inside the custom streambuf. But at the same time, it seems a bit weird that a class that could perfectly be used in a singled-threaded environment needs to always take care of synchronization?