diff --git a/CHANGELOG.md b/CHANGELOG.md index 001371ca3..62ca36e73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - Add `sentry_attachment_from_file/bytes` (and their wide-string variants) for creating attachment values that can be fully configured before they are added. ([#2079](https://github.com/getsentry/sentry-native/pull/2079)) - Add `sentry_add_attachment`, `sentry_scope_add_attachment`, and `sentry_hint_add_attachment` for adding configured attachments to the global scope, a specific scope, or a hint. These functions consume and freeze the attachment value. ([#2079](https://github.com/getsentry/sentry-native/pull/2079), [#1974](https://github.com/getsentry/sentry-native/pull/1974)) - Add `sentry_start_new_trace()` as a clearer name for starting a new trace. ([#2095](https://github.com/getsentry/sentry-native/pull/2095)) +- Add `sentry_scope_begin_read`/`sentry_scope_end_read` and `sentry_scope_begin_write`/`sentry_scope_end_write` for consistent reads of multiple scope properties and applying multiple scope changes with a single scope flush. ([#2107](https://github.com/getsentry/sentry-native/pull/2107)) **Fixes**: diff --git a/include/sentry.h b/include/sentry.h index 2516a8322..4800ecf9e 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -2544,6 +2544,55 @@ SENTRY_API sentry_scope_t *sentry_local_scope_new(void); */ SENTRY_API sentry_scope_t *sentry_scope_new(void); +/** + * Begins a batch read of `scope`. + * + * The scope remains read-locked until the matching `sentry_scope_end_read` call + * on the same thread, so multiple properties can be read consistently. Other + * threads may read the scope concurrently. Reads may be nested for the same + * scope, including inside a write batch. Mutating the scope inside an outermost + * read batch is not allowed. + * + * Returns 0 on success or 1 if `scope` is NULL or allocation fails. Each + * successful call must be matched by `sentry_scope_end_read` before freeing the + * scope. + */ +SENTRY_API int sentry_scope_begin_read(const sentry_scope_t *scope); + +/** + * Ends a read batch begun by `sentry_scope_begin_read`. + * + * The outermost call releases the scope read lock. Reads nested inside a write + * batch leave the scope write-locked until the outermost + * `sentry_scope_end_write`. This must be called on the same thread as the + * matching begin call. + */ +SENTRY_API void sentry_scope_end_read(const sentry_scope_t *scope); + +/** + * Begins a batch write of `scope`. + * + * The scope remains write-locked until the matching + * `sentry_scope_end_write` call on the same thread. Scope functions may be + * called normally during the batch, including functions that read the scope. + * Writes may be nested for the same scope, but an outermost read batch cannot + * be upgraded to a write batch. + * + * Returns 0 on success or 1 if `scope` is NULL, allocation fails, or a read + * batch would need upgrading. Each successful call must be matched by + * `sentry_scope_end_write` before freeing the scope. + */ +SENTRY_API int sentry_scope_begin_write(sentry_scope_t *scope); + +/** + * Ends a write batch begun by `sentry_scope_begin_write`. + * + * The outermost call releases the scope write lock and flushes any pending + * global scope changes to the backend. This must be called on the + * same thread as the matching begin call. + */ +SENTRY_API void sentry_scope_end_write(sentry_scope_t *scope); + /** * Frees a scope created via `sentry_scope_new`, `sentry_scope_clone`, or * `sentry_local_scope_new`. diff --git a/src/sentry_scope.c b/src/sentry_scope.c index 27c2c8dd6..957672f39 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -61,6 +61,13 @@ struct sentry_scope_data_s { bool trace_managed; }; +typedef struct scope_access_s { + sentry_scope_data_t *data; + size_t depth; + bool write; + struct scope_access_s *next; +} scope_access_t; + static bool g_scope_initialized = false; static sentry_scope_t g_scope = { 0 }; static sentry_scope_data_t g_scope_data = { 0 }; @@ -68,8 +75,10 @@ static bool g_scope_idle_initialized = false; static sentry_cond_t g_scope_idle; #ifdef _MSC_VER static __declspec(thread) size_t g_scope_depth; +static __declspec(thread) scope_access_t *g_scope_access; #else static __thread size_t g_scope_depth; +static __thread scope_access_t *g_scope_access; #endif #ifdef SENTRY__MUTEX_INIT_DYN SENTRY__MUTEX_INIT_DYN(g_lock) @@ -77,23 +86,79 @@ SENTRY__MUTEX_INIT_DYN(g_lock) static sentry_mutex_t g_lock = SENTRY__MUTEX_INIT; #endif +static scope_access_t * +scope_data_get_access(const sentry_scope_data_t *data) +{ +#ifndef SENTRY_PLATFORM_WINDOWS + // avoid dynamic TLS access during signal handling + if (!sentry__block_for_signal_handler()) { + return NULL; + } +#endif + for (scope_access_t *access = g_scope_access; access; + access = access->next) { + if (access->data == data) { + return access; + } + } + return NULL; +} + +static bool +scope_data_read_lock(const sentry_scope_data_t *data) +{ + if (scope_data_get_access(data)) { + return false; + } + sentry__rwlock_read_lock((sentry_rwlock_t *)&data->rwlock); + return true; +} + +static void +scope_data_read_unlock(const sentry_scope_data_t *data, bool locked) +{ + if (locked) { + sentry__rwlock_read_unlock((sentry_rwlock_t *)&data->rwlock); + } +} + +static bool +scope_data_write_lock(sentry_scope_data_t *data) +{ + scope_access_t *access = scope_data_get_access(data); + assert(!access || access->write); + if (access && access->write) { + return false; + } + sentry__rwlock_write_lock(&data->rwlock); + return true; +} + +static void +scope_data_write_unlock(sentry_scope_data_t *data, bool locked) +{ + if (locked) { + sentry__rwlock_write_unlock(&data->rwlock); + } +} + #define SENTRY_SCOPE_READ_LOCK(Data) \ for (const sentry_scope_data_t *_locked_data = (Data); _locked_data; \ - sentry__rwlock_read_unlock((sentry_rwlock_t *)&_locked_data->rwlock), \ - _locked_data = NULL) \ - for (bool _locked_once \ - = (sentry__rwlock_read_lock( \ - (sentry_rwlock_t *)&_locked_data->rwlock), \ - true); \ - _locked_once; _locked_once = false) + _locked_data = NULL) \ + for (bool _lock_acquired = scope_data_read_lock(_locked_data), \ + _locked_once = true; \ + _locked_once; \ + scope_data_read_unlock(_locked_data, _lock_acquired), \ + _locked_once = false) #define SENTRY_SCOPE_WRITE_LOCK(Data) \ for (sentry_scope_data_t *_locked_data = (Data); _locked_data; \ - sentry__rwlock_write_unlock(&_locked_data->rwlock), \ - _locked_data = NULL) \ - for (bool _locked_once \ - = (sentry__rwlock_write_lock(&_locked_data->rwlock), true); \ - _locked_once; _locked_once = false) + _locked_data = NULL) \ + for (bool _lock_acquired = scope_data_write_lock(_locked_data), \ + _locked_once = true; \ + _locked_once; \ + scope_data_write_unlock(_locked_data, _lock_acquired), \ + _locked_once = false) static size_t begin_scope_notify(sentry_scope_t *scope); static void end_scope_notify(sentry_scope_t *scope); @@ -533,33 +598,47 @@ sentry__scope_finish(sentry_scope_t *scope) sentry__scope_decref(scope); } -void -sentry__scope_finish_mut(sentry_scope_t *scope, bool flush) +static bool +scope_should_flush(sentry_scope_t *scope, bool flush) { - if (!scope) { - return; - } - sentry__mutex_lock(&scope->observers_lock); - if (scope->is_notifying > 0) { - // defer the flush requested by a reentrant scope change + scope_access_t *access = scope_data_get_access(scope->data); + if (scope->is_notifying > 0 || (access && access->write)) { + // defer the flush requested by a reentrant or batched scope change scope->pending_flush = flush || scope->pending_flush; flush = false; } else { - // consume any flush requested by a reentrant scope change + // consume any flush requested by a reentrant or batched scope change flush = flush || scope->pending_flush; scope->pending_flush = false; } sentry__mutex_unlock(&scope->observers_lock); + return flush; +} + +static void +flush_scope(void) +{ + SENTRY_WITH_OPTIONS (options) { + if (options->backend && options->backend->flush_scope_func) { + options->backend->flush_scope_func(options->backend, options); + } + } +} + +void +sentry__scope_finish_mut(sentry_scope_t *scope, bool flush) +{ + if (!scope) { + return; + } + + flush = scope_should_flush(scope, flush); sentry__scope_finish(scope); if (flush) { - SENTRY_WITH_OPTIONS (options) { - if (options->backend && options->backend->flush_scope_func) { - options->backend->flush_scope_func(options->backend, options); - } - } + flush_scope(); } } @@ -694,6 +773,102 @@ sentry_scope_new(void) return scope; } +static int +scope_begin(const sentry_scope_t *scope, bool write) +{ + if (!scope) { + return 1; + } + + sentry_scope_data_t *data = scope->data; + scope_access_t *access = scope_data_get_access(data); + if (access) { + if (write && !access->write) { + return 1; + } + access->depth++; + return 0; + } + + access = SENTRY_MAKE(scope_access_t); + if (!access) { + return 1; + } + + if (write) { + lock_scope_notify((sentry_scope_t *)scope); + sentry__rwlock_write_lock(&data->rwlock); + } else { + sentry__rwlock_read_lock(&data->rwlock); + } + access->data = data; + access->depth = 1; + access->write = write; + access->next = g_scope_access; + g_scope_access = access; + return 0; +} + +int +sentry_scope_begin_read(const sentry_scope_t *scope) +{ + return scope_begin(scope, false); +} + +int +sentry_scope_begin_write(sentry_scope_t *scope) +{ + return scope_begin(scope, true); +} + +static void +scope_end(const sentry_scope_t *scope) +{ + if (!scope) { + return; + } + + scope_access_t **link = &g_scope_access; + while (*link && (*link)->data != scope->data) { + link = &(*link)->next; + } + scope_access_t *access = *link; + if (!access) { + assert(false); + return; + } + if (--access->depth > 0) { + return; + } + + *link = access->next; + bool write = access->write; + sentry_free(access); + if (!write) { + sentry__rwlock_read_unlock(&scope->data->rwlock); + return; + } + sentry__rwlock_write_unlock(&scope->data->rwlock); + + bool flush = scope_should_flush((sentry_scope_t *)scope, scope == &g_scope); + unlock_scope_notify((sentry_scope_t *)scope); + if (flush && scope == &g_scope) { + flush_scope(); + } +} + +void +sentry_scope_end_read(const sentry_scope_t *scope) +{ + scope_end(scope); +} + +void +sentry_scope_end_write(sentry_scope_t *scope) +{ + scope_end(scope); +} + void sentry_scope_free(sentry_scope_t *scope) { diff --git a/tests/unit/test_scope.c b/tests/unit/test_scope.c index 118212bc6..9563ad65f 100644 --- a/tests/unit/test_scope.c +++ b/tests/unit/test_scope.c @@ -35,6 +35,85 @@ scope_value_get_length(scope_value_getter_t get, const sentry_scope_t *scope) return length; } +SENTRY_TEST(scope_write_nested) +{ + sentry_scope_t *scope = sentry_scope_new(); + TEST_ASSERT(!!scope); + + TEST_ASSERT(sentry_scope_begin_write(scope) == 0); + sentry_scope_set_tag(scope, "first", "value"); + + TEST_ASSERT(sentry_scope_begin_write(scope) == 0); + sentry_scope_set_tag(scope, "second", "value"); + TEST_ASSERT(sentry_scope_begin_read(scope) == 0); + TEST_CHECK(scope_value_get_length(sentry__scope_load_tags, scope) == 2); + sentry_scope_end_read(scope); + sentry_scope_end_write(scope); + + sentry_scope_set_environment(scope, "production"); + sentry_value_t environment = sentry__scope_ref_environment(scope); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(environment), "production"); + sentry_value_decref(environment); + sentry_scope_end_write(scope); + + TEST_CHECK(scope_value_get_length(sentry__scope_load_tags, scope) == 2); + sentry_scope_free(scope); +} + +typedef struct { + sentry_scope_t *scope; + sentry_waitable_flag_t started; + sentry_waitable_flag_t finished; + sentry_value_t environment; + sentry_value_t tag; + int result; +} scope_access_thread_t; + +SENTRY_THREAD_FN +read_scope_batch(void *data) +{ + scope_access_thread_t *state = data; + sentry__waitable_flag_set(&state->started); + state->result = sentry_scope_begin_read(state->scope); + if (state->result == 0) { + state->environment = sentry__scope_ref_environment(state->scope); + state->tag = scope_value_get_by_key( + sentry__scope_load_tags, state->scope, "version"); + sentry_scope_end_read(state->scope); + } + sentry__waitable_flag_set(&state->finished); + return 0; +} + +SENTRY_TEST(scope_write_exclusive) +{ + sentry_scope_t *scope = sentry_scope_new(); + TEST_ASSERT(!!scope); + + scope_access_thread_t state = { .scope = scope }; + sentry__waitable_flag_init(&state.started); + sentry__waitable_flag_init(&state.finished); + + TEST_ASSERT(sentry_scope_begin_write(scope) == 0); + sentry_scope_set_environment(scope, "new"); + sentry_threadid_t thread; + sentry__thread_init(&thread); + TEST_ASSERT(!sentry__thread_spawn(&thread, read_scope_batch, &state)); + TEST_ASSERT(sentry__waitable_flag_wait(&state.started, 1000)); + TEST_CHECK(!sentry__waitable_flag_wait(&state.finished, 10)); + + sentry_scope_set_tag(scope, "version", "new"); + sentry_scope_end_write(scope); + TEST_CHECK(sentry__waitable_flag_wait(&state.finished, 1000)); + sentry__thread_join(thread); + TEST_CHECK_INT_EQUAL(state.result, 0); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(state.environment), "new"); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(state.tag), "new"); + sentry_value_decref(state.environment); + sentry_value_decref(state.tag); + sentry_scope_free(scope); +} + SENTRY_TEST(scope_contexts) { SENTRY_TEST_OPTIONS_NEW(options); @@ -2060,6 +2139,23 @@ SENTRY_TEST(scope_observer_deferred_flush) TEST_CHECK_INT_EQUAL(observer_data.nested_flush_count, 0); TEST_CHECK_INT_EQUAL(observer_data.total_flush_count, 1); + observer_data.total_flush_count = 0; + observer_data.nested_flush_count = 0; + sentry_scope_t *scope = sentry__scope_getref(); + TEST_ASSERT(sentry_scope_begin_write(scope) == 0); + sentry_scope_set_tag(scope, "batched", "value"); + TEST_CHECK_INT_EQUAL(observer_data.nested_flush_count, 0); + TEST_CHECK_INT_EQUAL(observer_data.total_flush_count, 0); + TEST_ASSERT(sentry_scope_begin_read(scope) == 0); + sentry_scope_end_read(scope); + TEST_CHECK_INT_EQUAL(observer_data.total_flush_count, 0); + sentry_scope_end_write(scope); + TEST_CHECK_INT_EQUAL(observer_data.total_flush_count, 1); + TEST_ASSERT(sentry_scope_begin_read(scope) == 0); + sentry_scope_end_read(scope); + TEST_CHECK_INT_EQUAL(observer_data.total_flush_count, 1); + sentry__scope_finish(scope); + sentry_close(); } @@ -3464,3 +3560,120 @@ SENTRY_TEST(scope_clone_keeps_bound_span) sentry_close(); } + +SENTRY_TEST(scope_read_shared) +{ + sentry_scope_t *scope = sentry_scope_new(); + TEST_ASSERT(!!scope); + sentry_scope_set_environment(scope, "old"); + sentry_scope_set_tag(scope, "version", "old"); + + scope_access_thread_t state = { .scope = scope }; + sentry__waitable_flag_init(&state.started); + sentry__waitable_flag_init(&state.finished); + + TEST_ASSERT(sentry_scope_begin_read(scope) == 0); + sentry_threadid_t thread; + sentry__thread_init(&thread); + TEST_ASSERT(!sentry__thread_spawn(&thread, read_scope_batch, &state)); + TEST_CHECK(sentry__waitable_flag_wait(&state.finished, 1000)); + sentry_scope_end_read(scope); + sentry__thread_join(thread); + + TEST_CHECK_INT_EQUAL(state.result, 0); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(state.environment), "old"); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(state.tag), "old"); + sentry_value_decref(state.environment); + sentry_value_decref(state.tag); + sentry_scope_free(scope); +} + +SENTRY_THREAD_FN +write_scope_during_read(void *data) +{ + scope_access_thread_t *state = data; + sentry__waitable_flag_set(&state->started); + state->result = sentry_scope_begin_write(state->scope); + if (state->result == 0) { + sentry_scope_set_environment(state->scope, "new"); + sentry_scope_set_tag(state->scope, "version", "new"); + sentry_scope_end_write(state->scope); + } + sentry__waitable_flag_set(&state->finished); + return 0; +} + +SENTRY_TEST(scope_read_blocks_write) +{ + sentry_scope_t *scope = sentry_scope_new(); + TEST_ASSERT(!!scope); + sentry_scope_set_environment(scope, "old"); + sentry_scope_set_tag(scope, "version", "old"); + + scope_access_thread_t state = { .scope = scope }; + sentry__waitable_flag_init(&state.started); + sentry__waitable_flag_init(&state.finished); + + TEST_ASSERT(sentry_scope_begin_read(scope) == 0); + sentry_value_t environment = sentry__scope_ref_environment(scope); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(environment), "old"); + sentry_value_decref(environment); + + sentry_threadid_t thread; + sentry__thread_init(&thread); + TEST_ASSERT( + !sentry__thread_spawn(&thread, write_scope_during_read, &state)); + TEST_ASSERT(sentry__waitable_flag_wait(&state.started, 1000)); + TEST_CHECK(!sentry__waitable_flag_wait(&state.finished, 10)); + + TEST_ASSERT(sentry_scope_begin_read(scope) == 0); + sentry_value_t tag + = scope_value_get_by_key(sentry__scope_load_tags, scope, "version"); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(tag), "old"); + sentry_value_decref(tag); + TEST_CHECK(sentry_scope_begin_write(scope) == 1); + sentry_scope_end_read(scope); + TEST_CHECK(!sentry__waitable_flag_wait(&state.finished, 10)); + + sentry_scope_end_read(scope); + TEST_CHECK(sentry__waitable_flag_wait(&state.finished, 1000)); + sentry__thread_join(thread); + + TEST_CHECK_INT_EQUAL(state.result, 0); + environment = sentry__scope_ref_environment(scope); + tag = scope_value_get_by_key(sentry__scope_load_tags, scope, "version"); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(environment), "new"); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(tag), "new"); + sentry_value_decref(environment); + sentry_value_decref(tag); + sentry_scope_free(scope); +} + +SENTRY_TEST(scope_access_multiple) +{ + sentry_scope_t *first = sentry_scope_new(); + sentry_scope_t *second = sentry_scope_new(); + TEST_ASSERT(!!first && !!second); + + TEST_ASSERT(sentry_scope_begin_write(first) == 0); + TEST_ASSERT(sentry_scope_begin_read(second) == 0); + TEST_ASSERT(sentry_scope_begin_read(first) == 0); + TEST_ASSERT(sentry_scope_begin_write(first) == 0); + sentry_scope_set_tag(first, "key", "value"); + sentry_scope_end_write(first); + sentry_scope_end_read(first); + sentry_scope_end_write(first); + + TEST_CHECK(scope_value_get_length(sentry__scope_load_tags, second) == 0); + TEST_CHECK(sentry_scope_begin_write(second) == 1); + sentry_scope_end_read(second); + TEST_CHECK(scope_value_get_length(sentry__scope_load_tags, first) == 1); + + TEST_ASSERT(sentry_scope_begin_write(second) == 0); + sentry_scope_set_tag(second, "key", "value"); + sentry_scope_end_write(second); + TEST_CHECK(scope_value_get_length(sentry__scope_load_tags, second) == 1); + + sentry_scope_free(first); + sentry_scope_free(second); +} diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index 623e5bc97..b42dbc9e3 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -365,6 +365,7 @@ XX(rwlock_write_blocks_writer) XX(sampling_before_send) XX(sampling_decision) XX(sampling_transaction) +XX(scope_access_multiple) XX(scope_attachments) XX(scope_bind_span) XX(scope_bind_span_or_transaction_not_both) @@ -413,6 +414,8 @@ XX(scope_observer_transaction) XX(scope_observer_user) XX(scope_ownership) XX(scope_propagation_context) +XX(scope_read_blocks_write) +XX(scope_read_shared) XX(scope_rebind_same_object) XX(scope_release) XX(scope_remove_fingerprint_capture) @@ -425,6 +428,8 @@ XX(scope_transaction) XX(scope_update_context) XX(scope_user) XX(scope_user_id) +XX(scope_write_exclusive) +XX(scope_write_nested) XX(scoped_txn) XX(sentry__value_span_new_requires_unfinished_parent) XX(serialize_envelope)