Adding iostream.h thread-safety documentation.

Author: rwgk

docs/advanced/pycpp/utilities.rst

@@ -47,6 +47,18 @@ 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 a requirement that
+    all (possibly) concurrent redirected ostream writes are locked. Note
+    that this is not expected to be an actual limitation, because without
+    synchronization output will be randomly interleaved and most likely
+    unreadable. Well-written C++ code is likely to use locking regardless of
+    this pybind11 requirement. — For more background see the discussion under
+    `PR #2982 <https://github.com/pybind/pybind11/pull/2982>`_.
+
 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,13 @@
 
     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 a REQUIREMENT that all
+    (possibly) concurrent redirected ostream writes are locked. For more
+    background see the discussion under
+    https://github.com/pybind/pybind11/pull/2982.
 */
 
 #pragma once
@@ -85,30 +92,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

@@ -13,9 +13,8 @@
 
 #include <pybind11/iostream.h>
 #include "pybind11_tests.h"
-#include <atomic>
 #include <iostream>
-#include <thread>
+#include <string>
 
 void noisy_function(const std::string &msg, bool flush) {
 
@@ -29,40 +28,6 @@ void noisy_funct_dual(const std::string &msg, const std::string &emsg) {
     std::cerr << emsg;
 }
 
-// object to manage C++ thread
-// simply repeatedly write to std::cerr until stopped
-// redirect is called at some point to test the safety of scoped_estream_redirect
-struct TestThread {
-    TestThread() : stop_{false} {
-        auto thread_f = [this] {
-            while (!stop_) {
-                std::cout << "x" << std::flush;
-                std::this_thread::sleep_for(std::chrono::microseconds(50));
-            } };
-        t_ = new std::thread(std::move(thread_f));
-    }
-
-    ~TestThread() {
-        delete t_;
-    }
-
-    void stop() { stop_ = true; }
-
-    void join() const {
-        py::gil_scoped_release gil_lock;
-        t_->join();
-    }
-
-    void sleep() {
-        py::gil_scoped_release gil_lock;
-        std::this_thread::sleep_for(std::chrono::milliseconds(50));
-    }
-
-    std::thread *t_{nullptr};
-    std::atomic<bool> stop_;
-};
-
-
 TEST_SUBMODULE(iostream, m) {
 
     add_ostream_redirect(m);
@@ -104,10 +69,4 @@ TEST_SUBMODULE(iostream, m) {
         std::cout << msg << std::flush;
         std::cerr << emsg << std::flush;
     });
-
-    py::class_<TestThread>(m, "TestThread")
-        .def(py::init<>())
-        .def("stop", &TestThread::stop)
-        .def("join", &TestThread::join)
-        .def("sleep", &TestThread::sleep);
 }

tests/test_iostream.py

@@ -306,26 +306,3 @@ def test_redirect_both(capfd):
     assert stderr == ""
     assert stream.getvalue() == msg
     assert stream2.getvalue() == msg2
-
-
-def test_threading():
-    with m.ostream_redirect(stdout=True, stderr=False):
-        # start some threads
-        threads = []
-
-        # start some threads
-        for _j in range(20):
-            threads.append(m.TestThread())
-
-        # give the threads some time to fail
-        threads[0].sleep()
-
-        # stop all the threads
-        for t in threads:
-            t.stop()
-
-        for t in threads:
-            t.join()
-
-        # if a thread segfaults, we don't get here
-        assert True