From 2fa4747cd4bc8802a1f7c4344caa976ae8421082 Mon Sep 17 00:00:00 2001
From: nickbridgechess <74011726+nickbridgechess@users.noreply.github.com>
Date: Thu, 19 Nov 2020 05:09:33 -0600
Subject: [PATCH] pythonbuf fix (#2675)

* Added test_thread testing for ostream_redirect segfault recreation

* fix: scoped_ostream_redirect str created outside gil

* Moved threading tests into test_iostream. Cleaned up some formatting. Deleted test_thread.{cpp,py}

* CI: few formatting fixes

* CI: yet another formatting fix

* CI: more formatting fixes. Removed unecessary comment

* Ignore 'warning C4702: unreachable code' in MSVC 2015

Co-authored-by: Nick Bridge <nick.bridge.chess@gmail.com>
Co-authored-by: Nick Bridge <nbridge@jumptrading.com>
Co-authored-by: Yannick Jadoul <yannick.jadoul@belgacom.net>
---
 include/pybind11/iostream.h |  6 +++--
 tests/test_iostream.cpp     | 45 +++++++++++++++++++++++++++++++++++++
 tests/test_iostream.py      | 23 +++++++++++++++++++
 tests/test_smart_ptr.cpp    |  4 ++--
 4 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/include/pybind11/iostream.h b/include/pybind11/iostream.h
index 5e9a8143d..816e38f10 100644
--- a/include/pybind11/iostream.h
+++ b/include/pybind11/iostream.h
@@ -43,11 +43,13 @@ private:
     // simplified to a fully qualified call.
     int _sync() {
         if (pbase() != pptr()) {
-            // This subtraction cannot be negative, so dropping the sign
-            str line(pbase(), static_cast<size_t>(pptr() - pbase()));
 
             {
                 gil_scoped_acquire tmp;
+
+                // This subtraction cannot be negative, so dropping the sign.
+                str line(pbase(), static_cast<size_t>(pptr() - pbase()));
+
                 pywrite(line);
                 pyflush();
             }
diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp
index e9161505c..1be0655df 100644
--- a/tests/test_iostream.cpp
+++ b/tests/test_iostream.cpp
@@ -7,10 +7,15 @@
     BSD-style license that can be found in the LICENSE file.
 */
 
+#if defined(_MSC_VER) && _MSC_VER < 1910  // VS 2015's MSVC
+#  pragma warning(disable: 4702) // unreachable code in system header (xatomic.h(382))
+#endif
 
 #include <pybind11/iostream.h>
 #include "pybind11_tests.h"
+#include <atomic>
 #include <iostream>
+#include <thread>
 
 
 void noisy_function(std::string msg, bool flush) {
@@ -25,6 +30,40 @@ void noisy_funct_dual(std::string msg, 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] {
+            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() {
+        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<bool> stop_;
+};
+
+
 TEST_SUBMODULE(iostream, m) {
 
     add_ostream_redirect(m);
@@ -70,4 +109,10 @@ 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);
 }
diff --git a/tests/test_iostream.py b/tests/test_iostream.py
index 506db42ec..6d493beda 100644
--- a/tests/test_iostream.py
+++ b/tests/test_iostream.py
@@ -216,3 +216,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
diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp
index 60c2e692e..812b8e24c 100644
--- a/tests/test_smart_ptr.cpp
+++ b/tests/test_smart_ptr.cpp
@@ -8,8 +8,8 @@
     BSD-style license that can be found in the LICENSE file.
 */
 
-#if defined(_MSC_VER) && _MSC_VER < 1910
-#  pragma warning(disable: 4702) // unreachable code in system header
+#if defined(_MSC_VER) && _MSC_VER < 1910  // VS 2015's MSVC
+#  pragma warning(disable: 4702) // unreachable code in system header (xatomic.h(382))
 #endif
 
 #include "pybind11_tests.h"