-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-41435: Add sys._current_exceptions() function #21689
Conversation
This is a fantastic idea. I don't have time to review it but hopefully @serhiy-storchaka can have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that adding sys._current_exceptions()
is the best solution.
Python/pystate.c
Outdated
@@ -1195,6 +1195,66 @@ _PyThread_CurrentFrames(void) | |||
return result; | |||
} | |||
|
|||
/* The implementation of sys._current_exceptions(). This is intended to be | |||
called with the GIL held, as it will be when called via | |||
sys._current_frames(). It's possible it would work fine even without the GIL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have doubts that it would work fine without the GIL held because of: 1) auditing, 2) memory allocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copy/pasted the comment from sys._current_frames
since they use the same mechanism. I can also factor the code and the comment elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must not use the Python C API without holding the GIL: PyLong_FromUnsignedLong(), PyDict_SetItem(), etc. require to hold the GIL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we might want to update the comment for _current_frames
then!
454c7c3
to
789d9b0
Compare
What would you propose? The use case is real. |
a2d698c
to
e21084d
Compare
I do not know what the use case is exactly (I also do not know the use case of |
In my case, the use-case for both functions is to be able to do statistical profiling on programs and to learn which code paths are raising the most exceptions, which type of exceptions, etc. We do this currently in a hacky way with Cython and You can look at what Java and JFR do in this regard for example: https://stackoverflow.com/questions/60147519/how-to-profile-the-number-of-exceptions-generated-categorized-by-exception-clas |
@serhiy-storchaka Does that make a compelling argument? :) |
@vstinner any opinion on this? |
Lib/test/test_sys.py
Outdated
try: | ||
raise ValueError("oops") | ||
except ValueError: | ||
if leave_g.wait(timeout=0.1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to use support.SHORT_TIMEOUT or even support.LONG_TIMEOUT if the timeout should not occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
PyObject * | ||
_PyThread_CurrentExceptions(void) | ||
{ | ||
PyThreadState *tstate = _PyThreadState_GET(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to ensure that the GIL is held. For example, call _Py_EnsureTstateNotNULL(tstate).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Python/pystate.c
Outdated
@@ -1195,6 +1195,66 @@ _PyThread_CurrentFrames(void) | |||
return result; | |||
} | |||
|
|||
/* The implementation of sys._current_exceptions(). This is intended to be | |||
called with the GIL held, as it will be when called via | |||
sys._current_frames(). It's possible it would work fine even without the GIL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must not use the Python C API without holding the GIL: PyLong_FromUnsignedLong(), PyDict_SetItem(), etc. require to hold the GIL.
Python/sysmodule.c
Outdated
/*[clinic input] | ||
sys._current_exceptions | ||
|
||
Return a dict mapping each thread's thread id to its current raised exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-thread
=> thread's identifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if (err_info == NULL) { | ||
continue; | ||
} | ||
PyObject *id = PyLong_FromUnsignedLong(t->thread_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single thread can be associated to more than one Python thread state. It is possible that two interpreters run in the same thread, just not at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a limitation in _current_frames
then, right?
I think it'd be fine in this case to mimic what _current_frames
does in this regard.
.. function:: _current_exceptions() | ||
|
||
Return a dictionary mapping each thread's identifier to the topmost exception | ||
currently active in that thread at the time the function is called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear that threads with no exception are omitted in this dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
e21084d
to
62f48e2
Compare
eeed951
to
ae0e5d7
Compare
Python/pystate.c
Outdated
goto fail; | ||
} | ||
PyObject *exc_info = PyTuple_Pack(3, | ||
err_info->exc_type != NULL ? err_info->exc_type : Py_None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are too long. I propose to reduce indentation,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, re-indented.
This adds a new function named sys._current_exceptions() which is equivalent ot sys._current_frames() except that it returns the exceptions currently handled by other threads. It is equivalent to calling sys.exc_info() for each running thread.
ae0e5d7
to
0726d17
Compare
Can someone press the button? |
This adds a new function named sys._current_exceptions() which is equivalent ot sys._current_frames() except that it returns the exceptions currently handled by other threads. It is equivalent to calling sys.exc_info() for each running thread.
This adds a new function named sys._current_exceptions() which is equivalent ot
sys._current_frames() except that it returns the exceptions currently handled
by other threads. It is equivalent to calling sys.exc_info() for each running
thread.
https://bugs.python.org/issue41435