Fixes #1053: Use SET_ATOMIC_BOOL to set receive_complete flag on mess… #1054
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #1054 +/- ##
==========================================
- Coverage 30.96% 30.95% -0.01%
==========================================
Files 174 174
Lines 38181 38191 +10
Branches 5342 5345 +3
==========================================
Hits 11823 11823
- Misses 25204 25214 +10
Partials 1154 1154
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| content->receive_complete = true; | ||
| SET_ATOMIC_BOOL(&content->receive_complete, true); |
There was a problem hiding this comment.
Does this clear any tsan.supp entry, by any chance?
I noticed
skupper-router/tests/tsan.supp
Lines 35 to 36 in 81a196b
but looking at https://issues.apache.org/jira/browse/DISPATCH-2127, it does appear that atomically setting this flag will allow getting rid of the suppression.
There was a problem hiding this comment.
Thanks @jiridanek , I will take a look
There was a problem hiding this comment.
The removal of the suppression is dependent upon these three flags being accessed safely -
content->receive_complete
content->q2_input_holdoff
content->aborted
I have added additional code to this PR so that access to content->q2_input_holdoff is secured after holding content->lock
Let's see what the CI thinks.
There was a problem hiding this comment.
After removing the suppression, I see no new test failures on this PR. I am going to kick off the tests one more time to see what happens.
There was a problem hiding this comment.
Removing the suppression did not have any side effects. @jiridanek @kgiusti please take a look at this PR one more time and approve/comment.
src/message.c
Outdated
| @@ -2392,11 +2392,13 @@ qd_message_t *qd_message_compose(qd_composed_field_t *f1, | |||
| qd_compose_free(fields[idx]); | |||
| } | |||
|
|
|||
| LOCK(&content->lock); | |||
| // initialize the Q2 flag. Note that we do not need to hold the content | |||
There was a problem hiding this comment.
The comment is out-of-date, now?
There was a problem hiding this comment.
I don't think this comment is out-of-date. I agree with the comments and am inclined to remove the lock I added. @kgiusti can you please confirm ?
There was a problem hiding this comment.
Yeah this lock shouldn't be necessary IIUC because the message hasn't yet been shared with any other thread - it was just created in this function and we're just setting the q2_input_holdoff initial value.
Fixing the tsan warning was not the purpose of the original patch, which is to simply corrected a missing atomic operation. I'd recommend committing the original one-line change only without removing the tsan suppression since IIUC the suppression is still necessary after the one-line change is made.
There was a problem hiding this comment.
I believe that the suppression can be removed with this change. I will take out the lock that I added (since it is not necessary) and take out the suppression as well and see if this passed CI. if it does, the suppression can be safely removed
There was a problem hiding this comment.
The CI has passed. I am merging this PR to main, thanks.
…e flag on message content
ganeshmurthy
force-pushed
the
RECEIVE-COMPLETE
branch
from
e80c292
to
1551c46
Compare
…age content