Use critical section to protect pgconn ptr#1233
Conversation
d15da3c to
71c9fd7
Compare
ngoldbaum
left a comment
There was a problem hiding this comment.
Caught a couple minor issues. I was looking at the HTML annotated report from setting annotate=True in the cythonize call and making sure all the critical sections have no Python overhead:
diff --git a/psycopg_c/build_backend/psycopg_build_ext.py b/psycopg_c/build_backend/psycopg_build_ext.py
index b4293fe7..8d137304 100644
--- a/psycopg_c/build_backend/psycopg_build_ext.py
+++ b/psycopg_c/build_backend/psycopg_build_ext.py
@@ -63,5 +63,5 @@ class psycopg_build_ext(build_ext):
"always_allow_keywords": False,
"freethreading_compatible": True,
},
- annotate=False, # enable to get an html view of the C module
+ annotate=True, # enable to get an html view of the C module
)
My other big comment is that taking a critical section and then immediately doing with nogil is possibly an issue. Are the per-object locks getting released when the thread detaches from the interpreter? The critical section docs talk about "possibly blocking calls" and I'm not sure if that matters for the thread state APIs in the free-threaded build.
If the critical sections are getting released immediately, there's no reason to add them here and they'll give the reader a false sense of security. It may be the case that new locking is needed to make these APIs safe. Also given that the GIL is being released, if these APIs aren't safe to use from multiple threads, then it may be possible to cause data races on the GIL-enabled build right now.
dvarrazzo
left a comment
There was a problem hiding this comment.
The changes look ok, not invasive and they do the right thing w.r.t. managing the connection pointer.
Just asking to check if it's possible to use a couple of more modern idioms, for the rest the code seems good to go.
|
You might want to rebase this MR on master to run the CI successfully (macos 13 runners were dropped). |
Yes, the Cython docs also mention that and suggest not doing
|
Not just finish - all the spots where you're using If it really isn't safe to call into the If it's safe to call into If the desire is only to prevent new issues that aren't possible on the GIL-enabled build, then I think also the correct thing is to remove the new critical sections you've added here that do |
This PR adds critical sections to the PGconn class so that race condition between calling into the libpq bindings and calling `close` which sets the `_pgconn_ptr` to NULL are eliminated. Also add one high-level and one low-level tests that trigger TSAN warnings without this PR.
|
@dvarrazzo Friendly ping on this one. |
5f37ec8 to
d7dc6c7
Compare
|
Hello @lysnikolaou! I sincerely apologise for the delay, it had very busy weeks. Merged now, thank you very much! |
This PR adds critical sections to the PGconn class so that race condition between calling into the libpq bindings and calling
closewhich sets the_pgconn_ptrto NULL are eliminated.Also add one high-level and one low-level tests that trigger TSAN warnings without this PR.