[WIP] perf: use rwlock for scope data access - #1877
Conversation
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:
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Allow concurrent global scope readers while preserving recursive write semantics for existing SDK call paths.
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.
Add a non-recursive sentry_rwlock_t abstraction backed by platform reader/writer locks. Cover static and dynamic initialization, reader sharing, writer exclusion, and mixed read/write stress in unit tests.
Convert the POSIX rwlock operations from statement macros to static inline functions so callers can use them from scoped lock macro expressions.
Add a per-scope rwlock and use scoped read/write locking for simple by-value fields. Keep borrowed scope properties unchanged for now.
Add ref-style scope accessors for user and fingerprint values so callers get a retained reference that was incremented under the scope read lock. Guard user and fingerprint replacement with the scope write lock and keep observer notifications outside the locked section.
Store release, environment, and transaction as sentry_value_t fields with retained scope accessors. Pass value references through observer notifications and add a generic consuming sentry__value_replace helper for scope value replacement.
Move scope payload access behind sentry_scope_data_t helpers so scope-level code no longer reaches through scope->data to individual fields.
Since `sentry_value_new_string` calls `strlen` + `sentry_value_new_string_n` anyway, we might as well store the length. This does not only save us from inefficiently calling `strlen` later, but also implicitly adds support for embedded NUL bytes for presenting attachments (#1945).
495cd85 to
05fe336
Compare
jpnurmi
changed the title
| attachment, ATTACHMENT_PATH, sentry_value_new_string(path->path)); | ||
| set_filename_from_path(attachment, path); | ||
| sentry__path_free(path); | ||
| } |
There was a problem hiding this comment.
Path update clobbers filename
Medium Severity
sentry__attachment_set_path also rewrites filename from the path basename. Crashpad’s ensure_unique_path can suffix colliding names on disk; that used to update only path and keep the original logical filename for envelopes.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 05fe336. Configure here.
Track active scope accesses and delay cleanup until they finish, while releasing the lifetime guard before backend scope flushing.
Decrement owned values returned by attachment add helpers when callers only need the attachment list to retain them.
| SET(Key, sentry_value_new_string(Source)); \ | ||
| } \ | ||
| } while (0) | ||
| #define PLACE_STRING_VALUE(Key, Source) \ | ||
| do { \ | ||
| if (IS_NULL(Key) && sentry_value_get_length(Source) > 0) { \ |
There was a problem hiding this comment.
Bug: The release and environment fields are now explicitly set to null in the Dynamic Sampling Context (DSC) when not provided, instead of being omitted, which is a behavioral regression.
Severity: MEDIUM
Suggested Fix
Modify the logic that updates the Dynamic Sampling Context (DSC), such as in data_set_release, to check for null values. If the value is null, the key should be omitted from the DSC object rather than being set with a null value, restoring the previous behavior.
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_scope.c#L1750-L1755
Potential issue: When `options->release` or `options->environment` are `NULL`, the new
code in `sentry__scope_apply_options` sets the corresponding keys in the Dynamic
Sampling Context (DSC) to an explicit `null` value. Previously, these keys would have
been omitted from the DSC. This change in behavior can affect DSC serialization and any
downstream consumers that expect absent keys instead of `null` values. While event
application is protected by a check (`PLACE_STRING_VALUE`), the DSC is not, leading to
this regression.
|
|
||
| cleanup_data(data); | ||
| init_data(data); | ||
|
|
||
| sentry_value_decref(data->propagation_context); | ||
| sentry_value_decref(data->dynamic_sampling_context); | ||
| data->propagation_context = propagation_context; | ||
| data->dynamic_sampling_context = dynamic_sampling_context; | ||
| data->trace_managed = trace_managed; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: If breadcrumb ringbuffer allocation fails in clear_data due to OOM, the scope is left in a silently degraded state where breadcrumbs no longer function.
Severity: LOW
Suggested Fix
Check the return value of sentry__ringbuffer_new within init_data. If the allocation fails, the function should handle the error, possibly by attempting to restore the old state or by logging the failure, rather than silently leaving data->breadcrumbs as NULL.
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_scope.c#L253-L271
Potential issue: The `clear_data` function allocates a new breadcrumbs ringbuffer via
`sentry__ringbuffer_new` while holding a write lock. If this memory allocation fails
(e.g., under OOM conditions), `data->breadcrumbs` is set to `NULL` without any error
handling. Subsequent operations on breadcrumbs will silently fail as they are coded
defensively to handle a `NULL` ringbuffer, preventing a crash but leaving the scope in a
degraded state where breadcrumb functionality is lost without any notification.
| for (size_t i = 0; i < len; i++) { | ||
| sentry_value_t existing | ||
| = sentry_value_get_by_index(*attachments_ptr, i); | ||
| if (attachment_dedupe_eq(existing, attachment)) { | ||
| sentry_value_decref(attachment); | ||
| return sentry_value_incref(existing); | ||
| } | ||
|
|
||
| next_ptr = &it->next; | ||
| } | ||
|
|
||
| *next_ptr = attachment; | ||
| return attachment; | ||
| sentry_value_append(*attachments_ptr, attachment); | ||
| return sentry_value_get_by_index_owned(*attachments_ptr, len); |
There was a problem hiding this comment.
Bug: Attachments are frozen when added to the scope, which causes later modifications by backends (e.g., setting a path) to silently fail, leading to incorrect attachment data.
Severity: HIGH
Suggested Fix
Do not freeze attachments when they are added to the scope via sentry__attachments_add. Removing the call to sentry_value_freeze will allow backends to modify the attachment object as expected, for instance, to set the correct file path.
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_attachment.c#L471-L485
Potential issue: Attachments are frozen using `sentry_value_freeze` when added to the
scope. However, backend functions like `crashpad_backend_add_attachment` later attempt
to modify these frozen attachments by calling `sentry__attachment_set_path`. This
modification calls `sentry_value_set_by_key`, which silently fails on frozen objects. As
a result, the attachment's path is not updated, leading to incorrect path information or
data loss for attachments processed by backends.
Also affects:
src/backends/sentry_backend_crashpad.cpp:1107~1192src/backends/sentry_backend_native.c:929~968
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3c7c966. Configure here.
| // backend will do its own `WITH_SCOPE` internally. | ||
| sentry__mutex_unlock(&g_lock); | ||
| } else { | ||
| end_scope_access(); |
There was a problem hiding this comment.
Cleanup races with deferred scope flush
Medium Severity
finish_scope drops the scope access count before invoking flush_scope_func, but backends re-enter via SENTRY_WITH_SCOPE. That leaves a window where sentry__scope_cleanup can tear down or observe an idle scope, then the flush reinitializes an empty global scope after close or writes a blank crash snapshot.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3c7c966. Configure here.
1 participant
Warning
WIP 🚧🔨⏳⛔
Mostly curious what the AI review bots have to say for now...
Allow concurrent scope data readers.
Before
After
Close: #1862