[NOGIL] Make Message class methods thread safe - #2350
Ojasva Jain (ojasvajain) wants to merge 2 commits into
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
a916f2f to
24155f9
Compare
21bfa1f to
615744f
Compare
Kaushik Raina (k-raina)
left a comment
Kaushik Raina (k-raina)
left a comment
There was a problem hiding this comment.
Thanks for PR! Provided couple of comments
There was a problem hiding this comment.
Why are we not checking lock for "==" , "len()" etc?
There was a problem hiding this comment.
This got missed. I have added the checks.
There was a problem hiding this comment.
Can we expand more on test coverage, considering its change to message class? One thing i can thing of is memory leak test by counting refcount before and after?
There was a problem hiding this comment.
Added a few test cases related to reference counts.
|
Kaushik Raina (k-raina)
left a comment
Kaushik Raina (k-raina)
left a comment
There was a problem hiding this comment.
Thanks for PR Ojasva Jain (@ojasvajain)
Left couple of comments on correctness
Also. Are we making share consumer free threading safe?
|
|
||
| m = (Message *)msg; | ||
|
|
||
| if (m->error && m->error != Py_None) { |
There was a problem hiding this comment.
What happens when The standard consume loop, read a message, then commit it. While another thread touches that same message object?
|
|
||
| m = (Message *)msg; | ||
|
|
||
| if (m->error && m->error != Py_None) { |
There was a problem hiding this comment.
Same. What happens when manually storing an offset for a message while another thread touches it?
|
|
||
| /* One lock per mutable PyObject field above: serializes concurrent | ||
| * get/set of the same field on a shared Message (free-threaded). */ | ||
| cfl_lock_t topic_lock; |
There was a problem hiding this comment.
Do we need 5 locks accross message file? Is there scope to reduce number of locks?
2 participants
Message class exposes getters and setters to five fields. On free threaded Python builds, calling setters and getters concurrently can cause use-after-free conditions and incorrect reference counts.
This PR fixes this by acquiring a lock around code snippets where we want only one thread to run at a time (critical sections). The getters and setters have been refactored as part of this process. In Message_headers, an unrelated bug related to error not being handled is also fixed.
For the five fields that have getters and setters, we have introduced five lock fields within the Message struct.
Also contains a minor fix in Consumer_consume method.
Checklist
References
JIRA:
Test & Review
Open questions / Follow-ups