[FFI] Use the correct function in DecRef

[yelite]

Found this problem when investigating for #15996

cc @Lunderberg

[Lunderberg]

Oh my, that's a good catch, thank you! This would effectively mean that any python exception thrown across the FFI border would never be garbage-collected, and that any local variables in its stack trace would be in memory until the program exits. This is primarily used to improve debugging in cases where the exception is causing the program to exit anyways, but there's no guarantee that that's the case.

[yelite]

The fix in this PR exposes an actual problem of making C++ wrapper of a Python object. Running pytest tests/python/unittest/test_crt.py will result in error "the function must be called with the GIL held, but the GIL is released (the current Python thread state is NULL)".

In addition, I think we should also consider the stopping order of runtime here. After the Python runtime shuts down, DecRef can no longer be called.

@Lunderberg Since this goes far beyond my initial intention of fixing a typo, can you take this over and make proper fix?

[Lunderberg]

@yelite Certainly, and I have PR #16021 opened which includes both the typo fix and the GIL handling. It looks like it was an issue not with the underlying API usage, that the GIL wasn't explicitly acquired before using the python API. This wasn't an issue in single-threaded usage, because the GIL was held by the parent scope. It also didn't trigger an error previously, because the calls to Py_IncRef couldn't ever bring the reference count to zero.

Regarding stopping order, we should be set. The main concern would be the unspecified order of static destructors across compilation units, where both the CPython implementation and the last_error use static objects. However, the last_error field only stores a python object during the FFI handoff, and never retains it beyond that. For error propagation from Python to C++, it is set here in Python, then retrieved and re-thrown here in C++. For error propagation from C++ to Python, it is set here in C++, as part of the API_END() macro, and is then retrieved here in python. Since the last_error doesn't contain a wrapped python object when the static destructors are being called, we shouldn't have any ordering issues with CPython teardown.