From f443f9f4ed4897ca4fe916c3c8d3754640244cf7 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 6 May 2021 13:50:58 -0700 Subject: [PATCH 1/4] Adding iostream.h thread-safety documentation. --- docs/advanced/pycpp/utilities.rst | 12 +++++++++ include/pybind11/iostream.h | 42 ++++++++++++++++-------------- tests/test_iostream.cpp | 43 +------------------------------ tests/test_iostream.py | 23 ----------------- 4 files changed, 35 insertions(+), 85 deletions(-) diff --git a/docs/advanced/pycpp/utilities.rst b/docs/advanced/pycpp/utilities.rst index c15051fb96..f6ba0552bf 100644 --- a/docs/advanced/pycpp/utilities.rst +++ b/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 `_. + 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 diff --git a/include/pybind11/iostream.h b/include/pybind11/iostream.h index 89d1813ba8..466adf1d9d 100644 --- a/include/pybind11/iostream.h +++ b/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(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(remainder)); + // This subtraction cannot be negative, so dropping the sign. + auto size = static_cast(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(remainder)); } return 0; } diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index 908788007f..9546f62b9b 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -13,9 +13,8 @@ #include #include "pybind11_tests.h" -#include #include -#include +#include 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 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_(m, "TestThread") - .def(py::init<>()) - .def("stop", &TestThread::stop) - .def("join", &TestThread::join) - .def("sleep", &TestThread::sleep); } diff --git a/tests/test_iostream.py b/tests/test_iostream.py index e2b74d01cb..35cc5f8eab 100644 --- a/tests/test_iostream.py +++ b/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 From dcc3f63df3b2f796ea0d0ebe9737c94872ad5214 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 11 May 2021 00:52:08 -0700 Subject: [PATCH 2/4] Restoring `TestThread` code with added `std::lock_guard`. --- tests/test_iostream.cpp | 47 +++++++++++++++++++++++++++++++++++++++++ tests/test_iostream.py | 23 ++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index 9546f62b9b..74ca679c58 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -13,8 +13,11 @@ #include #include "pybind11_tests.h" +#include #include +#include #include +#include void noisy_function(const std::string &msg, bool flush) { @@ -28,6 +31,44 @@ 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() : t_{nullptr}, stop_{false} { + auto thread_f = [this] { + static std::mutex cout_mutex; + while (!stop_) { + { + const std::lock_guard 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)); + } + + ~TestThread() { + delete t_; + } + + void stop() { stop_ = true; } + + void join() { + 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_; + std::atomic stop_; +}; + + TEST_SUBMODULE(iostream, m) { add_ostream_redirect(m); @@ -69,4 +110,10 @@ TEST_SUBMODULE(iostream, m) { std::cout << msg << std::flush; std::cerr << emsg << std::flush; }); + + py::class_(m, "TestThread") + .def(py::init<>()) + .def("stop", &TestThread::stop) + .def("join", &TestThread::join) + .def("sleep", &TestThread::sleep); } diff --git a/tests/test_iostream.py b/tests/test_iostream.py index 35cc5f8eab..e2b74d01cb 100644 --- a/tests/test_iostream.py +++ b/tests/test_iostream.py @@ -306,3 +306,26 @@ 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 From 201013b6f4b64c63c22ad364f8102c74e79ac789 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 11 May 2021 01:14:21 -0700 Subject: [PATCH 3/4] Updating new comments to reflect new information. --- docs/advanced/pycpp/utilities.rst | 13 ++++++------- include/pybind11/iostream.h | 13 ++++++++----- tests/test_iostream.cpp | 6 ++++++ 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/docs/advanced/pycpp/utilities.rst b/docs/advanced/pycpp/utilities.rst index f6ba0552bf..8b8e5196af 100644 --- a/docs/advanced/pycpp/utilities.rst +++ b/docs/advanced/pycpp/utilities.rst @@ -51,13 +51,12 @@ redirects output to the corresponding Python streams: 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 `_. + 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 `_ and + `PR #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 diff --git a/include/pybind11/iostream.h b/include/pybind11/iostream.h index 466adf1d9d..c98067ad8a 100644 --- a/include/pybind11/iostream.h +++ b/include/pybind11/iostream.h @@ -7,11 +7,14 @@ 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. + 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 diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index 74ca679c58..79bc42d777 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -40,6 +40,12 @@ struct TestThread { static std::mutex cout_mutex; while (!stop_) { { + // #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 lock(cout_mutex); std::cout << "x" << std::flush; } From 307f8a8b5ab8d3f5230caf6ae33de514bdc789d4 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 3 Jul 2021 11:03:41 -0700 Subject: [PATCH 4/4] Fixing up `git rebase -X theirs` accidents. --- tests/test_iostream.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index 79bc42d777..c620b59493 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -35,7 +35,7 @@ void noisy_funct_dual(const std::string &msg, const std::string &emsg) { // 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() : t_{nullptr}, stop_{false} { + TestThread() : stop_{false} { auto thread_f = [this] { static std::mutex cout_mutex; while (!stop_) { @@ -60,7 +60,7 @@ struct TestThread { void stop() { stop_ = true; } - void join() { + void join() const { py::gil_scoped_release gil_lock; t_->join(); } @@ -70,7 +70,7 @@ struct TestThread { std::this_thread::sleep_for(std::chrono::milliseconds(50)); } - std::thread * t_; + std::thread *t_{nullptr}; std::atomic stop_; };