Skip to content

Commit 95df334

Browse files
committed
Reworks GIL related types to prevent some forms of program termination.
This change is a rework of some GIL related types in the library to prevent and avoid some unexpected crashes and program terminations, which would occur most often at process exit: - A new exception type named `InterpreterFinalizingException` is introduced. It is thrown by some functions in the library when an operation fails because the interpreter is currently finalizing. - `GILAcquire` constructor now throws a `InterpreterFinalizingException` when the interpreter is finalizing and therefore the GIL could not be acquired. - `GILRelease` destructor now does not reacquire the GIL when the interpreter is finalizing. - The `Guarded` and `GILGuardedObject` types are removed, and replaced with a `SharedObject` type. This type now wraps a Python object value as a shared reference-counted value that does not require the GIL to copy or move. The last copy of a `SharedObject` value will acquire the GIL to release the object, if possible, or do nothing (and leak it) if the GIL cannot be acquired. - The `DeleteInOtherThread` deleter type is removed. Its implementation was incorrect and it has become unneeded. - Destructors of `Application` and `ApplicationSession` do not use the `DeleterInOtherThread` deleter type anymore. The hypothesis is that the `DeleteInOtherThread` deleter was previously needed because `Application` values were wrapped in shared pointers and could be destroyed in callbacks executed by native threads. This is not the case anymore and instead, they destroy the application in the same thread that the Python `Application` object is garbage collected, which most likely is the interpreter main thread (due to `Application` being a singleton in the Python code, and its destruction is scheduled in an `atexit` callback). - A new `invokeCatchPythonError` is introduced. It acquires the GIL, then invokes a function with the given arguments, but catches any `pybind11::error_already_set` exception thrown during this invocation, and throws a `std::runtime_error` instead. - The `currentThreadHoldsGil` function now returns an optional denoting if the interpreter is initialized and not finalizing. Another function `gilExistsAndCurrentThreadHoldsIt` is introduced that returns just a bool. - The `ObjectDecRef` internal type is removed, as it is unused. This change mitigates the problem observed in issue #SW-4658.
1 parent f2e6d0a commit 95df334

12 files changed

Lines changed: 366 additions & 243 deletions

CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ target_sources(
115115

116116
PUBLIC
117117
qipython/common.hpp
118+
qipython/common.hxx
118119
qipython/pyapplication.hpp
119120
qipython/pyasync.hpp
120121
qipython/pyclock.hpp

qipython/common.hpp

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,28 @@ inline boost::optional<bool> interpreterIsFinalizing()
125125
#endif
126126
}
127127

128+
/// Acquires the GIL, then invokes a function with the given arguments, but
129+
/// catches any `pybind11::error_already_set` exception thrown during this
130+
/// invocation, and throws a `std::runtime_error` instead. This is useful to
131+
/// avoid the undefined behavior caused by the `what` member function of
132+
/// `pybind11::error_already_set` when the GIL is not acquired.
133+
template<typename F, typename... Args>
134+
auto invokeCatchPythonError(F&& f, Args&&... args);
135+
136+
/// Exception type thrown when an operation fails because the interpreter is
137+
/// finalizing.
138+
///
139+
/// Some operations are forbidden during interpreter finalization as they may
140+
/// terminate the thread they are called from. This exception allows stopping
141+
/// the flow of execution in this case before such functions are called.
142+
struct InterpreterFinalizingException : std::exception
143+
{
144+
inline const char* what() const noexcept override
145+
{
146+
return "the interpreter is finalizing";
147+
}
148+
};
149+
128150
} // namespace py
129151
} // namespace qi
130152

@@ -203,4 +225,6 @@ namespace detail
203225

204226
PYBIND11_DECLARE_HOLDER_TYPE(T, boost::shared_ptr<T>);
205227

228+
#include <qipython/common.hxx>
229+
206230
#endif // QIPYTHON_COMMON_HPP

qipython/common.hxx

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
** Copyright (C) 2023 Aldebaran Robotics
3+
** See COPYING for the license
4+
*/
5+
6+
#pragma once
7+
8+
#ifndef QIPYTHON_COMMON_HXX
9+
#define QIPYTHON_COMMON_HXX
10+
11+
#include <pybind11/pybind11.h>
12+
#include <qipython/common.hpp>
13+
#include <qipython/pyguard.hpp>
14+
15+
namespace qi
16+
{
17+
namespace py
18+
{
19+
20+
template<typename F, typename... Args>
21+
auto invokeCatchPythonError(F&& f, Args&&... args)
22+
{
23+
GILAcquire acquire;
24+
try
25+
{
26+
return std::invoke(std::forward<F>(f), std::forward<Args>(args)...);
27+
}
28+
catch (const pybind11::error_already_set& err)
29+
{
30+
throw std::runtime_error(err.what());
31+
}
32+
}
33+
34+
} // namespace py
35+
} // namespace qi
36+
37+
#endif // QIPYTHON_COMMON_HXX

0 commit comments

Comments
 (0)