While working on a (now cancelled) PR, I managed to provoke an obscure failure in PyImport_ImportModule by making PyEval_GetGlobals() consistent with PyEval_GetLocals() and having it set an error string when there was no current Python frame to read the globals from.
It turns out PyImport_ImportModule is assuming that PyEval_GetGlobals()won’t set an error when it returns NULL, so it doesn’t call PyErr_Clear(). Other consumers of this API could presumably be doing the same thing.
PyEval_GetLocals() setting an exception when it returns NULL dates from Python 3.4’s work on Issue 18408: Fixes crashes found by pyfailmalloc - Python tracker, where it gained the ability to report memory allocation failures when attempting to convert the fast locals array on optimised frames to a Python dictionary. Prior to that it worked the same way PyEval_GetGlobals() still does (NULL was just a possibly valid result indicating no Python frame was active, rather than an error indicator).
Given the complete lack of complaints about the Python 3.4 change, I’m leaning towards the following resolution:
document that PyEval_GetLocals() sets an exception when returning NULL
document that PyEval_GetGlobals() does NOT set an exception when returning NULL
add an embedding test case that enforces both of the above points and also ensures that PyImport_ImportModule() works when there’s no Python frame active
However, this thread exists because I’m wondering if the behaviour of PyEval_GetLocals() should be tweaked to a hybrid of its current behaviour and its pre-3.4 behaviour:
(status quo) locals available and successfully updated: return a pointer
(status quo) something went wrong reading the fast locals from an optimised frame: return NULL with an exception set
(revert to pre-3.4) no active Python frame: return NULL without setting an exception
I prefer moving towards API consistency with our other APIs where an exception is always set. So I like your hybrid better as it now changes it from never setting an exception (odd) to setting one at least in some circumstances so that we can document it as the caller needing to be aware than exception may have been set.
It is surprising for a Python C API call to not have an exception set on a NULL/-1 return value.
My C-API knowledge is extremely limited and I thus have no valid opinion on the change, but just to clarify, it sounds like @ncoghlan is saying the opposite here. Currently,
whereas
It appears that @ncoghlan is proposing the change the latter, not the former, to not set an exception when returning NULL if there is no active Python frame (unlike for Locals-specific memory allocation failures), consistent with the current behavior of PyEval_GetGlobals() (which would be left unchanged)
Right, prior to 3.4, neither function ever set an exception.
The subtlety is that when the runtime is embedded in another application, it isn’t an error for there to be no active Python frame, as you don’t get one of those until the thread starts running Python code.
+1. NULL return without an exception set is weird – the correct error handling is not obvious, so it ideally needs extra documentation (that people don’t read, especially when reviewing). I’d like to avoid it if possible, and keep the error handling that people need to do predictable.
(Yes, I added PyType_GetModuleState that does exactly this. Speaking from experience…)
There is one other antipattern here: PyEval_GetGlobals cannot raise, so users are tempted to skip the PyErr_Occurred check when they get NULL. This (generally) limits implementation details (in CPython and elsewhere) to code that always succeeds.
It’s probably not worth it to change PyEval_GetGlobals, but I wouldn’t revert PyEval_GetLocals to the pre-3.4 behavior.
I don’t get that. Sure, there might not be an active frame, but why shouldn’t asking for locals be an error in that case?
Returning NULL from these APIs for “no frame active” is akin to returning None from an API in Python - the expectation is that the caller either knows there is an active frame, or has a fallback if there isn’t one (e.g. module imports just skip the checks that look inside the importing module’s globals).
Since Python 3.4 PyEval_GetLocals() has been the odd API out in these frame related APIs: still documented as returning NULL if no frame is active, but unlike the rest of them, setting an exception when it does so.
Look at the docs, this isn’t “one API behaves one way, one API behaves another”, it’s “several APIs behave one way, with a single odd API out”: Reflection — Python 3.10.4 documentation
Yes, PyEval_GetLocals is inconsistent with the rest on that page, but it’s consistent with most other C API.
Of course, when it comes to consistency, it’s more important to be consist within the smaller group. But it’s not just about consistency here. PyEval_GetLocals is also the better API.
When users get NULL, do they also use PyErr_Occurred to distinguish the “None” NULL from a MemoryError? Should they?
IMO, they should – otherwise NULL is like returning None and also masking possible exceptions. But I’d be surprised if many did it.
They shouldn’t need to - it’s safe to call PyErr_Clear() when no error is set, so PyEval_GetLocals() returned NULL is still an adequate check to decide whether to call PyErr_Clear() or not. However, that also means there’s no real advantage to skipping setting the exception in the “no frame” case, since callers still need to account for the possible exception from the optimised frame handling.
That means the hybrid API behaviour doesn’t offer any meaningful benefits over the current API, so “status quo wins a stalemate” applies, and I’ll just add the missing :versionchanged: 3.4 note to the PyEval_GetLocals() docs at the same time as I add the embedding test case that ensures PyImport_ImportModule() works without an already active Python frame.
