Detach thread context during long-running operations#39
Conversation
The underlying Assuan calls are not thread safe and we can't perform multiple operations on the same Assuan context simultaneously. Additionally, the possibility of code calling into this extension from multiple threads necessiatates data race protection mechanisms. After introducing the necessary protections, I'm moderately confident that this package is now compatible with "free-threaded" Python interpreters as well, so we might as well declare so.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 48.97% 49.00% +0.02%
==========================================
Files 20 20
Lines 2638 2704 +66
Branches 538 543 +5
==========================================
+ Hits 1292 1325 +33
- Misses 1039 1063 +24
- Partials 307 316 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| rc = assuan_socket_connect(self->ctx, sockpath, ASSUAN_INVALID_PID, 0); | ||
| g_free(sockpath); | ||
| if (rc) { | ||
| assuan_release(self->ctx); |
There was a problem hiding this comment.
| assuan_release(self->ctx); | |
| assuan_release(self->ctx); | |
| self->ctx = NULL; |
|
|
||
| if (NULL != self->thread) { | ||
| g_thread_ref(self->thread); | ||
| g_thread_join(self->thread); |
There was a problem hiding this comment.
This introduces a two join calls in the same thread, see https://github.com/osrf/createrepo-agent/pull/39/changes#diff-7adef503c4ab0fd9ea6a7492ca9a95dd96ae7c31de069278a6e0834e90abe9e9R172 . Probably unsafe?
|
|
||
| Py_BEGIN_ALLOW_THREADS | ||
|
|
||
| g_rw_lock_writer_lock(&self->lock); |
There was a problem hiding this comment.
Might be a deadlock here? I don't see the unlock call before the Py_END_ALLOW_THREADS.
|
|
||
| g_rw_lock_reader_unlock(&self->lock); | ||
|
|
||
| g_rw_lock_writer_lock(&self->lock); |
There was a problem hiding this comment.
Do this also need to check in serer if active like the check above for the reader_lock?
| Py_RETURN_NONE; | ||
| } | ||
|
|
||
| self->sentinel = 1; |
There was a problem hiding this comment.
Does this write operation needs to be under the writer lock?
3 participants
The underlying Assuan calls are not thread safe and we can't perform multiple operations on the same Assuan context simultaneously. Additionally, the possibility of code calling into this extension from multiple threads necessiatates data race protection mechanisms.
After introducing the necessary protections, I'm moderately confident that this package is now compatible with "free-threaded" Python interpreters as well, so we might as well declare so.