Skip to content

Commit

Permalink
guard refcnt for nogil python
Browse files Browse the repository at this point in the history
  • Loading branch information
marscher committed Nov 18, 2024
1 parent 50fa58d commit 7c31d02
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 1 deletion.
8 changes: 7 additions & 1 deletion native/common/jp_tracer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,13 @@ void JPypeTracer::tracePythonObject(const char* msg, PyObject* ref)
if (ref != nullptr)
{
std::stringstream str;
str << msg << " " << (void*) ref << " " << Py_REFCNT(ref) << " " << Py_TYPE(ref)->tp_name;

This comment has been minimized.

Copy link
@Thrameos

Thrameos Nov 18, 2024

Contributor

Perhaps it would be cleaner to make a GET_REF macro in the header file an then just use it throughout the code.

str << msg << " " << (void*) ref << " "<<
#ifndef Py_GIL_DISABLED
Py_REFCNT(ref)
#else
-1
#endif
<< " " << Py_TYPE(ref)->tp_name;
JPypeTracer::trace1("PY", str.str().c_str());

} else
Expand Down
4 changes: 4 additions & 0 deletions native/python/jp_pythontypes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

static void assertValid(PyObject *obj)
{
#ifndef Py_GIL_DISABLED
if (Py_REFCNT(obj) >= 1)
return;

Expand All @@ -34,6 +35,9 @@ static void assertValid(PyObject *obj)
JP_TRACE_PY("pyref FAULT", obj);
JP_RAISE(PyExc_SystemError, "Deleted reference");
// GCOVR_EXCL_STOP
#else
return; // GIL is disabled; we assume obj is valid

This comment has been minimized.

Copy link
@astrelsky

astrelsky Nov 20, 2024

Contributor

@marscher this can be problematic as I have gotten the "Deleted reference" error before. What is the reason for not using Py_REFCNT when built with no gil? The correct handling is done in Python already so I don't understand why you would just skip this when it is disabled.

This comment has been minimized.

Copy link
@marscher

marscher Nov 24, 2024

Author Member

The expanded macro leads to an undefined name in threaded Python. I should have documented this error for future (proper) handling.

But you're totally right, this is a drastic change in behavior. Could you please raise a new issue about it?

#endif
}

/**
Expand Down

0 comments on commit 7c31d02

Please sign in to comment.