GH-93503: Add thread-specific APIs to set profiling and tracing functions in the C-API #93504
Conversation
pablogsal
changed the title
|
I think this is too error prone, for a couple of reasons.
To be fair, this isn't just an issue with these functions, but any function that takes a #93503 is about adding an API to set tracing/profiling on all threads. Also, I don't like offering operations in the C-API that aren't available in Python. We shouldn't be forcing people to write C. |
I am fine with that. Do you want me to update this PR or create another one?
That ship already sailed. Do mean that you would like to also expose such an API in the Python layer? |
|
The threading module has setprofile() and settrace() functions. |
But those only work for newly created threads as I noted in the issue. They are used only when new threads are bootstrapped. What we are trying to do is to add APIs that affect all current threads. We also cannot change those for backwards compatibility, but we could place the new APIs in the thread module for Python access. |
|
@markshannon you ok with this plan?
|
Yes. Sounds good to me. |
Would it make sense to add a parameter to these threading functions or to sys functions to apply the new tracing/profiling functions to existing threads? |
|
I am ambivalent, but the new parameter sounds good if you prefer it that way. |
|
Ok, I have made a first version of the idea proposed by @markshannon. Please @vstinner @markshannon review again. |
I suggest all_threads=True rather than running_threads=True: in your implementation, threading.settrace() changes old and new threads, all threads, not only old ("running") threads.
With the PR, we have:
- sys.settrace(): current thread
- threading.settrace(): new threads
- threading.settrace(all_threads=True): all threads (old, new and current)
|
@vstinner I'm going to change this PR to follow @markshannon request and make it a separate function in Python instead of a parameter. |
Sure, as you want. What would be the API in that case? |
|
I have updated the APIs to use separate functions. |
|
| @@ -7033,6 +7033,20 @@ PyEval_SetProfile(Py_tracefunc func, PyObject *arg) | |||
| } | |||
| } | |||
|
|
|||
| void | |||
There was a problem hiding this comment.
It's not great that the function cannot report errors. While not returning -1 on error and pass the exception to the caller, rather than handling it with _PyErr_WriteUnraisableMsg()?
There was a problem hiding this comment.
I am mimicking what we do already in PyEval_SetProfile and to avoid surprises I think we should keep these two as synchronized as possible.
There was a problem hiding this comment.
We cannot fix the API of old functions, but we can avoid past mistakes in newly added functions.
There was a problem hiding this comment.
If you want to ignore exceptions on purpose, please mention it in the function documentation.
There was a problem hiding this comment.
If you want to ignore exceptions on purpose, please mention it in the function documentation.
Yeah, I want to do that because I don't want to stop setting the profile function on other threads if an exception in one fails. Unless you strongly disagree, I will document this.
|
I plan to land this this week or the next one to avoid merge conflicts. @markshannon can you give it a last pass? |
|
Nice enhancement, thanks. |
|
You should document the newly added C API functions to: https://docs.python.org/dev/whatsnew/3.12.html#c-api-changes You should also document the new threading functions in the same document, in a threading section, near: https://docs.python.org/dev/whatsnew/3.12.html#improved-modules |
|
closes: #93503