From e980e3a797816475d00c7fa148f5909cb22cbe68 Mon Sep 17 00:00:00 2001 From: Nick Bridge Date: Thu, 5 Nov 2020 15:51:37 -0600 Subject: [PATCH 1/7] Added test_thread testing for ostream_redirect segfault recreation --- tests/CMakeLists.txt | 3 ++- tests/test_thread.cpp | 53 +++++++++++++++++++++++++++++++++++++++++++ tests/test_thread.py | 28 +++++++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 tests/test_thread.cpp create mode 100644 tests/test_thread.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index dae8b5ad43..8571a6ac6c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -122,7 +122,8 @@ set(PYBIND11_TEST_FILES test_stl_binders.cpp test_tagbased_polymorphic.cpp test_union.cpp - test_virtual_functions.cpp) + test_virtual_functions.cpp + test_thread.cpp) # Invoking cmake with something like: # cmake -DPYBIND11_TEST_OVERRIDE="test_callbacks.cpp;test_pickling.cpp" .. diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp new file mode 100644 index 0000000000..83798a71e1 --- /dev/null +++ b/tests/test_thread.cpp @@ -0,0 +1,53 @@ +/* + tests/test_tread.cpp -- thread handling + + Copyright (c) 2020 Nick Bridge + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ + +#include "pybind11_tests.h" +#include +#include +#include +#include + + +TEST_SUBMODULE(test_threading, m) { + // 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( !this->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_ = 1; } + 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_; + }; + + py::class_(m, "TestThread") + .def(py::init<>()) + .def("stop", &TestThread::stop) + .def("join", &TestThread::join) + .def("sleep", &TestThread::sleep); + +} + diff --git a/tests/test_thread.py b/tests/test_thread.py new file mode 100644 index 0000000000..740168bc3b --- /dev/null +++ b/tests/test_thread.py @@ -0,0 +1,28 @@ +# -*- coding: utf-8 -*- +import pytest +from pybind11_tests import test_threading as m +from pybind11_tests import iostream +import time + +def test_threading(): + with iostream.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 30e1eece1e17ee7f7e439e293923401f5af86257 Mon Sep 17 00:00:00 2001 From: Nick Bridge Date: Thu, 5 Nov 2020 17:07:32 -0600 Subject: [PATCH 2/7] fix: scoped_ostream_redirect str created outside gil --- include/pybind11/iostream.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/include/pybind11/iostream.h b/include/pybind11/iostream.h index 5e9a8143d0..532d0ecbd7 100644 --- a/include/pybind11/iostream.h +++ b/include/pybind11/iostream.h @@ -43,11 +43,15 @@ class pythonbuf : public std::streambuf { // 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(pptr() - pbase())); { gil_scoped_acquire tmp; + + // This subtraction cannot be negative, so dropping the sign + // this invokes python and therefore should be included in the + // GIL + str line(pbase(), static_cast(pptr() - pbase())); + pywrite(line); pyflush(); } From 7d58920ed0138a3d175f5bfacb478aa1115aba0b Mon Sep 17 00:00:00 2001 From: Nick Bridge Date: Wed, 18 Nov 2020 13:13:25 -0600 Subject: [PATCH 3/7] Moved threading tests into test_iostream. Cleaned up some formatting. Deleted test_thread.{cpp,py} --- include/pybind11/iostream.h | 4 +-- tests/CMakeLists.txt | 3 +-- tests/test_iostream.cpp | 45 +++++++++++++++++++++++++++++++ tests/test_iostream.py | 23 ++++++++++++++++ tests/test_thread.cpp | 53 ------------------------------------- tests/test_thread.py | 28 -------------------- 6 files changed, 71 insertions(+), 85 deletions(-) delete mode 100644 tests/test_thread.cpp delete mode 100644 tests/test_thread.py diff --git a/include/pybind11/iostream.h b/include/pybind11/iostream.h index 532d0ecbd7..70d022fdb1 100644 --- a/include/pybind11/iostream.h +++ b/include/pybind11/iostream.h @@ -47,8 +47,8 @@ class pythonbuf : public std::streambuf { { gil_scoped_acquire tmp; - // This subtraction cannot be negative, so dropping the sign - // this invokes python and therefore should be included in the + // This subtraction cannot be negative, so dropping the sign. + // This invokes python and therefore should be included in the // GIL str line(pbase(), static_cast(pptr() - pbase())); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8571a6ac6c..dae8b5ad43 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -122,8 +122,7 @@ set(PYBIND11_TEST_FILES test_stl_binders.cpp test_tagbased_polymorphic.cpp test_union.cpp - test_virtual_functions.cpp - test_thread.cpp) + test_virtual_functions.cpp) # Invoking cmake with something like: # cmake -DPYBIND11_TEST_OVERRIDE="test_callbacks.cpp;test_pickling.cpp" .. diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index e9161505ca..38faf008d2 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -11,6 +11,8 @@ #include #include "pybind11_tests.h" #include +#include +#include void noisy_function(std::string msg, bool flush) { @@ -25,6 +27,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( !this->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_ = 1; } + + 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); @@ -70,4 +106,13 @@ 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 506db42ec1..dc63b643cf 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_thread.cpp b/tests/test_thread.cpp deleted file mode 100644 index 83798a71e1..0000000000 --- a/tests/test_thread.cpp +++ /dev/null @@ -1,53 +0,0 @@ -/* - tests/test_tread.cpp -- thread handling - - Copyright (c) 2020 Nick Bridge - - All rights reserved. Use of this source code is governed by a - BSD-style license that can be found in the LICENSE file. -*/ - -#include "pybind11_tests.h" -#include -#include -#include -#include - - -TEST_SUBMODULE(test_threading, m) { - // 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( !this->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_ = 1; } - 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_; - }; - - py::class_(m, "TestThread") - .def(py::init<>()) - .def("stop", &TestThread::stop) - .def("join", &TestThread::join) - .def("sleep", &TestThread::sleep); - -} - diff --git a/tests/test_thread.py b/tests/test_thread.py deleted file mode 100644 index 740168bc3b..0000000000 --- a/tests/test_thread.py +++ /dev/null @@ -1,28 +0,0 @@ -# -*- coding: utf-8 -*- -import pytest -from pybind11_tests import test_threading as m -from pybind11_tests import iostream -import time - -def test_threading(): - with iostream.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 4e766cbb275cc3906515a2e8147033bafecf6c9d Mon Sep 17 00:00:00 2001 From: Nick Bridge Date: Wed, 18 Nov 2020 13:20:13 -0600 Subject: [PATCH 4/7] CI: few formatting fixes --- tests/test_iostream.cpp | 2 +- tests/test_iostream.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index 38faf008d2..196b975095 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -33,7 +33,7 @@ void noisy_funct_dual(std::string msg, std::string emsg) { struct TestThread { TestThread() : t_{nullptr}, stop_{false} { auto thread_f = [this] { - while( !this->stop_ ) { + while(!this->stop_) { std::cout << "x" << std::flush; std::this_thread::sleep_for(std::chrono::microseconds(50)); } }; diff --git a/tests/test_iostream.py b/tests/test_iostream.py index dc63b643cf..6d493beda3 100644 --- a/tests/test_iostream.py +++ b/tests/test_iostream.py @@ -224,8 +224,8 @@ def test_threading(): threads = [] # start some threads - for j_ in range(20): - threads.append( m.TestThread() ) + for _j in range(20): + threads.append(m.TestThread()) # give the threads some time to fail threads[0].sleep() From d22c20fcdc588589198bca2d25073d1e420e6930 Mon Sep 17 00:00:00 2001 From: Nick Bridge Date: Wed, 18 Nov 2020 13:26:49 -0600 Subject: [PATCH 5/7] CI: yet another formatting fix --- tests/test_iostream.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index 196b975095..6df73dcb23 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -33,7 +33,7 @@ void noisy_funct_dual(std::string msg, std::string emsg) { struct TestThread { TestThread() : t_{nullptr}, stop_{false} { auto thread_f = [this] { - while(!this->stop_) { + while (!stop_) { std::cout << "x" << std::flush; std::this_thread::sleep_for(std::chrono::microseconds(50)); } }; From 99ca9aaec0cf8f50f2ad763cd8f9bb297cb1c27f Mon Sep 17 00:00:00 2001 From: Nick Bridge Date: Wed, 18 Nov 2020 13:33:46 -0600 Subject: [PATCH 6/7] CI: more formatting fixes. Removed unecessary comment --- include/pybind11/iostream.h | 2 -- tests/test_iostream.cpp | 5 +---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/include/pybind11/iostream.h b/include/pybind11/iostream.h index 70d022fdb1..816e38f10b 100644 --- a/include/pybind11/iostream.h +++ b/include/pybind11/iostream.h @@ -48,8 +48,6 @@ class pythonbuf : public std::streambuf { gil_scoped_acquire tmp; // This subtraction cannot be negative, so dropping the sign. - // This invokes python and therefore should be included in the - // GIL str line(pbase(), static_cast(pptr() - pbase())); pywrite(line); diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index 6df73dcb23..f76f30588d 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -10,9 +10,9 @@ #include #include "pybind11_tests.h" +#include #include #include -#include void noisy_function(std::string msg, bool flush) { @@ -109,10 +109,7 @@ TEST_SUBMODULE(iostream, m) { py::class_(m, "TestThread") .def(py::init<>()) - .def("stop", &TestThread::stop) - .def("join", &TestThread::join) - .def("sleep", &TestThread::sleep); } From 483d501d26ee5ba406565fd0c134727ed4713ab5 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Wed, 18 Nov 2020 22:10:57 +0100 Subject: [PATCH 7/7] Ignore 'warning C4702: unreachable code' in MSVC 2015 --- tests/test_iostream.cpp | 5 ++++- tests/test_smart_ptr.cpp | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index f76f30588d..1be0655dfb 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -7,6 +7,9 @@ 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 #include "pybind11_tests.h" @@ -44,7 +47,7 @@ struct TestThread { delete t_; } - void stop() { stop_ = 1; } + void stop() { stop_ = true; } void join() { py::gil_scoped_release gil_lock; diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 60c2e692e5..812b8e24c5 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"