Skip to content

Commit ea56d1e

Browse files
pitroupitrou
committed
Fix PythonErrorDetail to store Python error state
(and restore it in check_status())
1 parent 21e1b95 commit ea56d1e

File tree

8 files changed

+239
-128
lines changed

8 files changed

+239
-128
lines changed

‎cpp/src/arrow/python/common.cc‎

Lines changed: 118 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,15 @@
2323

2424
#include "arrow/memory_pool.h"
2525
#include "arrow/status.h"
26+
#include "arrow/util/checked_cast.h"
2627
#include "arrow/util/logging.h"
2728

2829
#include "arrow/python/helpers.h"
2930

3031
namespace arrow {
32+
33+
using internal::checked_cast;
34+
3135
namespace py {
3236

3337
static std::mutex memory_pool_mutex;
@@ -48,22 +52,128 @@ MemoryPool* get_memory_pool() {
4852
}
4953

5054
// ----------------------------------------------------------------------
51-
// PythonErroDetail
55+
// PythonErrorDetail
56+
5257
namespace {
5358

54-
const char kErrorDetailTypeId[] = "existing exception on python static status.";
55-
// PythonErrorDetail indicates a python exception was raised and not
56-
// reset in C++ code (i.e. it should be propagated up through the python
57-
// stack).
59+
const char kErrorDetailTypeId[] = "arrow::py::PythonErrorDetail";
60+
61+
// Try to match the Python exception type with an appropriate Status code
62+
StatusCode MapPyError(PyObject* exc_type) {
63+
StatusCode code;
64+
65+
if (PyErr_GivenExceptionMatches(exc_type, PyExc_MemoryError)) {
66+
code = StatusCode::OutOfMemory;
67+
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_IndexError)) {
68+
code = StatusCode::IndexError;
69+
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_KeyError)) {
70+
code = StatusCode::KeyError;
71+
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_TypeError)) {
72+
code = StatusCode::TypeError;
73+
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_ValueError) ||
74+
PyErr_GivenExceptionMatches(exc_type, PyExc_OverflowError)) {
75+
code = StatusCode::Invalid;
76+
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_EnvironmentError)) {
77+
code = StatusCode::IOError;
78+
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_NotImplementedError)) {
79+
code = StatusCode::NotImplemented;
80+
} else {
81+
code = StatusCode::UnknownError;
82+
}
83+
return code;
84+
}
85+
86+
// PythonErrorDetail indicates a Python exception was raised.
5887
class PythonErrorDetail : public StatusDetail {
5988
public:
60-
PythonErrorDetail() = default;
6189
const char* type_id() const override { return kErrorDetailTypeId; }
62-
std::string ToString() const override { return "Uncaught Python."; }
90+
91+
std::string ToString() const override {
92+
// This is simple enough not to need the GIL
93+
const auto ty = reinterpret_cast<const PyTypeObject*>(exc_type_.obj());
94+
// XXX Should we also print traceback?
95+
return std::string("Python exception: ") + ty->tp_name;
96+
}
97+
98+
void RestorePyError() const {
99+
Py_INCREF(exc_type_.obj());
100+
Py_INCREF(exc_value_.obj());
101+
Py_INCREF(exc_traceback_.obj());
102+
PyErr_Restore(exc_type_.obj(), exc_value_.obj(), exc_traceback_.obj());
103+
}
104+
105+
PyObject* exc_type() const { return exc_type_.obj(); }
106+
107+
PyObject* exc_value() const { return exc_value_.obj(); }
108+
109+
static std::shared_ptr<PythonErrorDetail> FromPyError() {
110+
PyObject* exc_type = nullptr;
111+
PyObject* exc_value = nullptr;
112+
PyObject* exc_traceback = nullptr;
113+
114+
PyErr_Fetch(&exc_type, &exc_value, &exc_traceback);
115+
PyErr_NormalizeException(&exc_type, &exc_value, &exc_traceback);
116+
ARROW_CHECK(exc_type)
117+
<< "PythonErrorDetail::FromPyError called without a Python error set";
118+
DCHECK(PyType_Check(exc_type));
119+
DCHECK(exc_value); // Ensured by PyErr_NormalizeException, double-check
120+
if (exc_traceback == nullptr) {
121+
// Needed by PyErr_Restore()
122+
Py_INCREF(Py_None);
123+
exc_traceback = Py_None;
124+
}
125+
126+
std::shared_ptr<PythonErrorDetail> detail(new PythonErrorDetail);
127+
detail->exc_type_.reset(exc_type);
128+
detail->exc_value_.reset(exc_value);
129+
detail->exc_traceback_.reset(exc_traceback);
130+
return detail;
131+
}
132+
133+
protected:
134+
PythonErrorDetail() = default;
135+
136+
OwnedRefNoGIL exc_type_, exc_value_, exc_traceback_;
63137
};
64138

65139
} // namespace
66140

141+
// ----------------------------------------------------------------------
142+
// Python exception <-> Status
143+
144+
Status ConvertPyError(StatusCode code) {
145+
auto detail = PythonErrorDetail::FromPyError();
146+
if (code == StatusCode::UnknownError) {
147+
code = MapPyError(detail->exc_type());
148+
}
149+
150+
std::string message;
151+
RETURN_NOT_OK(internal::PyObject_StdStringStr(detail->exc_value(), &message));
152+
return Status(code, message, detail);
153+
}
154+
155+
Status PassPyError() {
156+
if (PyErr_Occurred()) {
157+
return ConvertPyError();
158+
}
159+
return Status::OK();
160+
}
161+
162+
bool IsPyError(const Status& status) {
163+
if (status.ok()) {
164+
return false;
165+
}
166+
auto detail = status.detail();
167+
bool result = detail != nullptr && detail->type_id() == kErrorDetailTypeId;
168+
return result;
169+
}
170+
171+
void RestorePyError(const Status& status) {
172+
ARROW_CHECK(IsPyError(status));
173+
const auto& detail = checked_cast<const PythonErrorDetail&>(*status.detail());
174+
detail.RestorePyError();
175+
}
176+
67177
// ----------------------------------------------------------------------
68178
// PyBuffer
69179

@@ -81,8 +191,7 @@ Status PyBuffer::Init(PyObject* obj) {
81191
}
82192
return Status::OK();
83193
} else {
84-
auto detail = std::make_shared<PythonErrorDetail>();
85-
return Status(StatusCode::Invalid, "", detail);
194+
return ConvertPyError(StatusCode::Invalid);
86195
}
87196
}
88197

@@ -101,68 +210,5 @@ PyBuffer::~PyBuffer() {
101210
}
102211
}
103212

104-
// ----------------------------------------------------------------------
105-
// Python exception -> Status
106-
107-
Status ConvertPyError(StatusCode code) {
108-
PyObject* exc_type = nullptr;
109-
PyObject* exc_value = nullptr;
110-
PyObject* traceback = nullptr;
111-
112-
PyErr_Fetch(&exc_type, &exc_value, &traceback);
113-
PyErr_NormalizeException(&exc_type, &exc_value, &traceback);
114-
115-
DCHECK_NE(exc_type, nullptr) << "ConvertPyError called without an exception set";
116-
117-
OwnedRef exc_type_ref(exc_type);
118-
OwnedRef exc_value_ref(exc_value);
119-
OwnedRef traceback_ref(traceback);
120-
121-
std::string message;
122-
RETURN_NOT_OK(internal::PyObject_StdStringStr(exc_value, &message));
123-
124-
if (code == StatusCode::UnknownError) {
125-
// Try to match the Python exception type with an appropriate Status code
126-
if (PyErr_GivenExceptionMatches(exc_type, PyExc_MemoryError)) {
127-
code = StatusCode::OutOfMemory;
128-
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_IndexError)) {
129-
code = StatusCode::IndexError;
130-
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_KeyError)) {
131-
code = StatusCode::KeyError;
132-
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_TypeError)) {
133-
code = StatusCode::TypeError;
134-
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_ValueError) ||
135-
PyErr_GivenExceptionMatches(exc_type, PyExc_OverflowError)) {
136-
code = StatusCode::Invalid;
137-
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_EnvironmentError)) {
138-
code = StatusCode::IOError;
139-
} else if (PyErr_GivenExceptionMatches(exc_type, PyExc_NotImplementedError)) {
140-
code = StatusCode::NotImplemented;
141-
}
142-
}
143-
144-
// We consume the python error here so don't construct with a detail.
145-
return Status(code, message);
146-
}
147-
148-
Status PassPyError() {
149-
if (PyErr_Occurred()) {
150-
// Do not call PyErr_Clear, the assumption is that someone further
151-
// up the call stack will want to deal with the Python error.
152-
auto detail = std::make_shared<PythonErrorDetail>();
153-
return Status(StatusCode::UnknownError, "", detail);
154-
}
155-
return Status::OK();
156-
}
157-
158-
bool IsPythonError(const Status& status) {
159-
if (status.ok()) {
160-
return false;
161-
}
162-
auto detail = status.detail();
163-
bool result = detail != nullptr && detail->type_id() == kErrorDetailTypeId;
164-
return result;
165-
}
166-
167213
} // namespace py
168214
} // namespace arrow

‎cpp/src/arrow/python/common.h‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,15 @@ class Result;
3636

3737
namespace py {
3838

39+
// Convert current Python error to a Status. The Python error state is cleared
40+
// and can be restored with RestorePyError().
3941
ARROW_PYTHON_EXPORT Status ConvertPyError(StatusCode code = StatusCode::UnknownError);
42+
// Same as ConvertPyError(), but returns Status::OK() if no Python error is set.
43+
ARROW_PYTHON_EXPORT Status PassPyError();
44+
// Query whether the given Status is a Python error (as wrapped by ConvertPyError()).
45+
ARROW_PYTHON_EXPORT bool IsPyError(const Status& status);
46+
// Restore a Python error wrapped in a Status.
47+
ARROW_PYTHON_EXPORT void RestorePyError(const Status& status);
4048

4149
// Catch a pending Python exception and return the corresponding Status.
4250
// If no exception is pending, Status::OK() is returned.
@@ -48,10 +56,6 @@ inline Status CheckPyError(StatusCode code = StatusCode::UnknownError) {
4856
}
4957
}
5058

51-
ARROW_PYTHON_EXPORT Status PassPyError();
52-
ARROW_PYTHON_EXPORT bool IsPythonError(const Status& status);
53-
54-
// TODO(wesm): We can just let errors pass through. To be explored later
5559
#define RETURN_IF_PYERROR() ARROW_RETURN_NOT_OK(CheckPyError());
5660

5761
#define PY_RETURN_IF_ERROR(CODE) ARROW_RETURN_NOT_OK(CheckPyError(CODE));
@@ -98,6 +102,18 @@ class ARROW_PYTHON_EXPORT PyAcquireGIL {
98102
ARROW_DISALLOW_COPY_AND_ASSIGN(PyAcquireGIL);
99103
};
100104

105+
// A RAII-style helper that releases the GIL until the end of a lexical block
106+
class ARROW_PYTHON_EXPORT PyReleaseGIL {
107+
public:
108+
PyReleaseGIL() { saved_state_ = PyEval_SaveThread(); }
109+
110+
~PyReleaseGIL() { PyEval_RestoreThread(saved_state_); }
111+
112+
private:
113+
PyThreadState* saved_state_;
114+
ARROW_DISALLOW_COPY_AND_ASSIGN(PyReleaseGIL);
115+
};
116+
101117
// A helper to call safely into the Python interpreter from arbitrary C++ code.
102118
// The GIL is acquired, and the current thread's error status is preserved.
103119
template <typename Function>
@@ -110,7 +126,7 @@ Status SafeCallIntoPython(Function&& func) {
110126
Status st = std::forward<Function>(func)();
111127
// If the return Status is a "Python error", the current Python error status
112128
// describes the error and shouldn't be clobbered.
113-
if (!IsPythonError(st) && exc_type != NULLPTR) {
129+
if (!IsPyError(st) && exc_type != NULLPTR) {
114130
PyErr_Restore(exc_type, exc_value, exc_traceback);
115131
}
116132
return st;

0 commit comments

Comments
 (0)