gh-154431: Use existing runtime->audit_hooks.mutex instead of a new one#154494
gh-154431: Use existing runtime->audit_hooks.mutex instead of a new one#154494sobolevn wants to merge 1 commit into
runtime->audit_hooks.mutex instead of a new one#154494Conversation
ZeroIntensity
left a comment
There was a problem hiding this comment.
This will just increase contention in sys.addaudithook, won't it?
|
I don't think that we actually concurrenly add hooks with both |
|
Agreed, but why make this change at all? We're just increasing pressure on a lock for no reason. |
|
Because we can't backport the fix with interpreter state changed due to ABI change. |
|
|
||
| PyInterpreterState *interp = tstate->interp; | ||
| PyMutex mutex = interp->audit_hooks_mutex; | ||
| PyMutex mutex = interp->runtime->audit_hooks.mutex; |
There was a problem hiding this comment.
Hi, I noticed the mutex here is copied by value onto the stack, then being locked. I'm worried this subtly changes the semantics into locking a stack-local object instead. The typical usage may be to lock the address directly, i.e. PyMutex_Lock(&interp->runtime->audit_hooks.mutex);
There was a problem hiding this comment.
Here is a demo shows locking a stack-local mutex won't affect the real mutex.
#include <Python.h>
#include <stdio.h>
static PyMutex shared = {0};
int main(void)
{
PyMutex mutex = shared;
PyMutex_Lock(&mutex);
printf("locked the COPY: shared=%d copy=%d\n", PyMutex_IsLocked(&shared), PyMutex_IsLocked(&mutex));
PyMutex_Unlock(&mutex);
PyMutex_Lock(&shared);
printf("locked the FIELD: shared=%d\n", PyMutex_IsLocked(&shared));
PyMutex_Unlock(&shared);
return 0;
}Run gcc mutex_copy_demo.c -I Include -I . libpython3.16.a -lpthread -lm -ldl -lutil -o demo && ./demo then it print
locked the COPY: shared=0 copy=1
locked the FIELD: shared=1
sys.addaudithook()lazily createsinterp->audit_hookswith no lock #154431