Restore reader_count in case of failure in rwlock_rdlock_impl#3053
Restore reader_count in case of failure in rwlock_rdlock_impl#3053shanhe72101 wants to merge 1 commit intoapache:masterfrom
Conversation
|
LGTM |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the read-write lock implementation where the reader count is not properly decremented when acquiring a read lock fails. The issue occurs in the rwlock_rdlock_impl function when bthread_sem_timedwait returns an error, leaving the reader count in an inconsistent state.
- Adds proper cleanup of reader count on lock acquisition failure
- Applies the fix consistently across all code paths in the function
- Maintains thread-safe atomic operations for reader count manipulation
| if (NULL == bthread::g_cp) { | ||
| return bthread_sem_timedwait(&rwlock->reader_sema, abstime); | ||
| int rc = bthread_sem_timedwait(&rwlock->reader_sema, abstime); | ||
| if (rc != 0){ |
There was a problem hiding this comment.
[nitpick] Missing space before opening brace. Should be 'if (rc != 0) {' to follow consistent formatting style.
| if (rc != 0){ | |
| if (rc != 0) { |
| if (!bvar::is_sampling_range_valid(sampling_range)) { // Don't sample. | ||
| return bthread_sem_timedwait(&rwlock->reader_sema, abstime); | ||
| int rc = bthread_sem_timedwait(&rwlock->reader_sema, abstime); | ||
| if (rc != 0){ |
There was a problem hiding this comment.
[nitpick] Missing space before opening brace. Should be 'if (rc != 0) {' to follow consistent formatting style.
| if (rc != 0){ | |
| if (rc != 0) { |
| const int64_t start_ns = butil::cpuwide_time_ns(); | ||
| int rc = bthread_sem_timedwait(&rwlock->reader_sema, abstime); | ||
| const int64_t end_ns = butil::cpuwide_time_ns(); | ||
| if (rc != 0){ |
There was a problem hiding this comment.
[nitpick] Missing space before opening brace. Should be 'if (rc != 0) {' to follow consistent formatting style.
| if (rc != 0){ | |
| if (rc != 0) { |
shanhe72101
force-pushed
the
fix_rwlock_rdlock_impl
branch
from
db53bb8 to
82d7cec
Compare
done |
| "bthread_key_unittest.cpp", | ||
| "bthread_butex_multi_tag_unittest.cpp", | ||
| "bthread_rwlock_unittest.cpp", | ||
| #"bthread_rwlock_unittest.cpp", |
There was a problem hiding this comment.
Is there any specific reason why this line is commented out?
There was a problem hiding this comment.
The unit test for rwlock is located in bthread_rwlock_unittest.cpp. I need to compile and run this test. In fact, I don't understand why bthread_rwlock_unittest.cpp was excluded.
There was a problem hiding this comment.
@shanhe72101 It appears that the bthread_rwlock_unittest.cpp file originally existed here, but your PR has commented out this test file. Is this as expected?
There was a problem hiding this comment.
@lorinlee "bthread_rwlock_unittest.cpp" is in the exclude list; you'll understand by checking the full configuration of the build target "bthread_test".
|
@chenBright please review,thank you |
chenBright
left a comment
chenBright
left a comment
There was a problem hiding this comment.
reader_semashould also be restored.- The same problem exists with
wlock.
In case of timeout, reader_sema will not be modified and does not require restoration. The same applies to rwlock->writer_sema. |
bthread1 bthread2
bthread_sem_timedwait
rwlock->reader_count->fetch_add(RWLockMaxReaders);
bthread_sem_post_n
bthread_mutex_unlock
rwlock->reader_count->fetch_addIn this case, |
5 participants
What problem does this PR solve?
Issue Number: Fix #3051
Problem Summary:
What is changed and the side effects?
Changed:
Side effects:
Performance effects:
Breaking backward compatibility:
Check List: