[WIP] perf: use recursive rwlock for global scope#1877
Conversation
Allow concurrent global scope readers while preserving recursive write semantics for existing SDK call paths.
jpnurmi
changed the title
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- use recursive rwlock for global scope ([#1877](https://github.com/getsentry/sentry-native/pull/1877))If none of the above apply, you can opt out of this check by adding |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1877 +/- ##
==========================================
+ Coverage 72.66% 75.67% +3.01%
==========================================
Files 85 90 +5
Lines 14698 21168 +6470
Branches 2512 3761 +1249
==========================================
+ Hits 10680 16019 +5339
- Misses 3704 4325 +621
- Partials 314 824 +510 🚀 New features to boost your workflow:
|
Block new readers while writers are queued and use separate condition variables so writer wake-ups cannot be consumed by waiting readers. Add coverage ensuring a queued writer runs before later readers.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 495cd85. Configure here.
| lock->readers++; | ||
| sentry__cond_wake(&lock->reader_cond); | ||
| sentry__mutex_unlock(&lock->mutex); | ||
| } |
There was a problem hiding this comment.
Nested reads deadlock under writers
High Severity
sentry__rwlock_read_lock treats writer-owned nesting as recursive, but a non-writer that already holds a read lock can block again when waiting_writers > 0. That thread still owns readers, so a waiting writer never proceeds and nested SENTRY_WITH_SCOPE (or read-then-write) deadlocks. The old recursive mutex allowed these nests; rwlock_recursion only covers the no-writer case.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 495cd85. Configure here.
| { | ||
| g_scope_flush_pending = true; | ||
| sentry__scope_write_unlock(); | ||
| } |
There was a problem hiding this comment.
NO_FLUSH can still flush
Medium Severity
sentry__scope_write_unlock flushes whenever g_scope_flush_pending is set on the outermost unlock. SENTRY_WITH_SCOPE_MUT_NO_FLUSH uses that path, so an inner SENTRY_WITH_SCOPE_MUT can force a flush on the outer NO_FLUSH exit. That breaks the no-flush contract used by paths like sentry_add_breadcrumb.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 495cd85. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| lock->writer_depth++; | ||
| sentry__mutex_unlock(&lock->mutex); | ||
| return; |
There was a problem hiding this comment.
Bug: The sentry__rwlock_read_lock function doesn't check for waiting writers, allowing new readers to acquire the lock while a writer is queued, causing indefinite writer starvation.
Severity: HIGH
Suggested Fix
Modify sentry__rwlock_read_lock to block new readers while writers are queued. The wait condition should check for both active writers and waiting writers, for example: while (lock->writer_depth > 0 || lock->waiting_writers > 0).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry_sync.h#L458-L460
Potential issue: The `sentry__rwlock_read_lock` function does not check for
`waiting_writers` before granting a read lock. When no writer holds the lock
(`writer_depth == 0`) but one or more writers are queued (`waiting_writers > 0`), new
readers can bypass the wait condition and increment `readers`. This keeps the waiting
writer's condition (`while (lock->writer_depth > 0 || lock->readers > 0)`) from ever
being met under a continuous read load, causing indefinite writer starvation. As a
result, SDK calls that write scope data, such as `sentry_set_user` or `sentry_set_tag`,
can be starved indefinitely in high-throughput scenarios.
Did we get this right? 👍 / 👎 to inform future reviews.
1 participant
Warning
WIP 🚧🔨⏳⛔
Mostly curious what the AI review bots have to say for now...
Allow concurrent global scope readers while preserving recursive write semantics for existing SDK call paths.
Before
After
Close: #1862