Adding iostream.h thread-safety documentation. (#2995)

* Adding iostream.h thread-safety documentation.

* Restoring `TestThread` code with added `std::lock_guard<std::mutex>`.

* Updating new comments to reflect new information.

* Fixing up `git rebase -X theirs` accidents.

Author: rwgk

docs/advanced/pycpp/utilities.rst

@@ -47,6 +47,17 @@ redirects output to the corresponding Python streams:
         call_noisy_func();
     });
 
+.. warning::
+
+    The implementation in ``pybind11/iostream.h`` is NOT thread safe. Multiple
+    threads writing to a redirected ostream concurrently cause data races
+    and potentially buffer overflows. Therefore it is currrently a requirement
+    that all (possibly) concurrent redirected ostream writes are protected by
+    a mutex. #HelpAppreciated: Work on iostream.h thread safety. For more
+    background see the discussions under
+    `PR #2982 <https://github.com/pybind/pybind11/pull/2982>`_ and
+    `PR #2995 <https://github.com/pybind/pybind11/pull/2995>`_.
+
 This method respects flushes on the output streams and will flush if needed
 when the scoped guard is destroyed. This allows the output to be redirected in
 real time, such as to a Jupyter notebook. The two arguments, the C++ stream and

include/pybind11/iostream.h

@@ -5,6 +5,16 @@
 
     All rights reserved. Use of this source code is governed by a
     BSD-style license that can be found in the LICENSE file.
+
+    WARNING: The implementation in this file is NOT thread safe. Multiple
+    threads writing to a redirected ostream concurrently cause data races
+    and potentially buffer overflows. Therefore it is currrently a requirement
+    that all (possibly) concurrent redirected ostream writes are protected by
+    a mutex.
+    #HelpAppreciated: Work on iostream.h thread safety.
+    For more background see the discussions under
+    https://github.com/pybind/pybind11/pull/2982 and
+    https://github.com/pybind/pybind11/pull/2995.
 */
 
 #pragma once
@@ -85,30 +95,25 @@ class pythonbuf : public std::streambuf {
         return remainder;
     }
 
-    // This function must be non-virtual to be called in a destructor. If the
-    // rare MSVC test failure shows up with this version, then this should be
-    // simplified to a fully qualified call.
+    // This function must be non-virtual to be called in a destructor.
     int _sync() {
         if (pbase() != pptr()) { // If buffer is not empty
             gil_scoped_acquire tmp;
-            // Placed inside gil_scoped_acquire as a mutex to avoid a race.
-            if (pbase() != pptr()) { // Check again under the lock
-                // This subtraction cannot be negative, so dropping the sign.
-                auto size        = static_cast<size_t>(pptr() - pbase());
-                size_t remainder = utf8_remainder();
-
-                if (size > remainder) {
-                    str line(pbase(), size - remainder);
-                    pywrite(line);
-                    pyflush();
-                }
-
-                // Copy the remainder at the end of the buffer to the beginning:
-                if (remainder > 0)
-                    std::memmove(pbase(), pptr() - remainder, remainder);
-                setp(pbase(), epptr());
-                pbump(static_cast<int>(remainder));
+            // This subtraction cannot be negative, so dropping the sign.
+            auto size        = static_cast<size_t>(pptr() - pbase());
+            size_t remainder = utf8_remainder();
+
+            if (size > remainder) {
+                str line(pbase(), size - remainder);
+                pywrite(line);
+                pyflush();
             }
+
+            // Copy the remainder at the end of the buffer to the beginning:
+            if (remainder > 0)
+                std::memmove(pbase(), pptr() - remainder, remainder);
+            setp(pbase(), epptr());
+            pbump(static_cast<int>(remainder));
         }
         return 0;
     }

tests/test_iostream.cpp

@@ -15,6 +15,8 @@
 #include "pybind11_tests.h"
 #include <atomic>
 #include <iostream>
+#include <mutex>
+#include <string>
 #include <thread>
 
 void noisy_function(const std::string &msg, bool flush) {
@@ -35,8 +37,18 @@ void noisy_funct_dual(const std::string &msg, const std::string &emsg) {
 struct TestThread {
     TestThread() : stop_{false} {
         auto thread_f = [this] {
+            static std::mutex cout_mutex;
             while (!stop_) {
-                std::cout << "x" << std::flush;
+                {
+                    // #HelpAppreciated: Work on iostream.h thread safety.
+                    // Without this lock, the clang ThreadSanitizer (tsan) reliably reports a
+                    // data race, and this test is predictably flakey on Windows.
+                    // For more background see the discussion under
+                    // https://github.com/pybind/pybind11/pull/2982 and
+                    // https://github.com/pybind/pybind11/pull/2995.
+                    const std::lock_guard<std::mutex> lock(cout_mutex);
+                    std::cout << "x" << std::flush;
+                }
                 std::this_thread::sleep_for(std::chrono::microseconds(50));
             } };
         t_ = new std::thread(std::move(thread_f));