diff --git a/.clang-format b/.clang-format index 56b761bb2..ba4b564d9 100644 --- a/.clang-format +++ b/.clang-format @@ -4,4 +4,4 @@ IndentPPDirectives: AfterHash ColumnLimit: 80 AlwaysBreakAfterDefinitionReturnType: All PointerAlignment: Right -ForEachMacros: ['SENTRY_WITH_SCOPE', 'SENTRY_WITH_SCOPE_MUT', 'SENTRY_WITH_SCOPE_MUT_NO_FLUSH', 'SENTRY_WITH_OPTIONS'] +ForEachMacros: ['SENTRY_WITH_SCOPE', 'SENTRY_WITH_SCOPE_MUT', 'SENTRY_WITH_SCOPE_MUT_NO_FLUSH', 'SENTRY_WITH_OPTIONS', 'SENTRY_WITH_OPTIONS_MUT'] diff --git a/src/sentry_core.c b/src/sentry_core.c index ba1b54c69..56f8e66b2 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -36,6 +36,19 @@ sentry__options_getref(void) return options; } +sentry_options_t * +sentry__options_lock(void) +{ + sentry__mutex_lock(&g_options_lock); + return g_options; +} + +void +sentry__options_unlock(void) +{ + sentry__mutex_unlock(&g_options_lock); +} + static void load_user_consent(sentry_options_t *opts) { @@ -72,6 +85,12 @@ sentry__should_skip_upload(void) int sentry_init(sentry_options_t *options) { + // this function is to be called only once, so we do not allow more than one + // caller + sentry__mutex_lock(&g_options_lock); + // pre-init here, so we can consistently use bailing out to :fail + sentry_transport_t *transport = NULL; + sentry_close(); sentry_logger_t logger = { NULL, NULL }; @@ -84,11 +103,10 @@ sentry_init(sentry_options_t *options) if (sentry__path_create_dir_all(options->database_path)) { SENTRY_WARN("failed to create database directory or there is no write " "access to this directory"); - sentry_options_free(options); - return 1; + goto fail; } - sentry_transport_t *transport = options->transport; + transport = options->transport; sentry_path_t *database_path = options->database_path; options->database_path = sentry__path_absolute(database_path); if (options->database_path) { @@ -139,9 +157,7 @@ sentry_init(sentry_options_t *options) last_crash = backend->get_last_crash_func(backend); } - sentry__mutex_lock(&g_options_lock); g_options = options; - sentry__mutex_unlock(&g_options_lock); // *after* setting the global options, trigger a scope and consent flush, // since at least crashpad needs that. @@ -167,6 +183,7 @@ sentry_init(sentry_options_t *options) sentry_start_session(); } + sentry__mutex_unlock(&g_options_lock); return 0; fail: @@ -175,21 +192,21 @@ sentry_init(sentry_options_t *options) sentry__transport_shutdown(transport, 0); } sentry_options_free(options); + sentry__mutex_unlock(&g_options_lock); return 1; } int sentry_close(void) { - sentry_end_session(); - + // this function is to be called only once, so we do not allow more than one + // caller sentry__mutex_lock(&g_options_lock); sentry_options_t *options = g_options; - g_options = NULL; - sentry__mutex_unlock(&g_options_lock); size_t dumped_envelopes = 0; if (options) { + sentry_end_session(); if (options->backend && options->backend->shutdown_func) { SENTRY_TRACE("shutting down backend"); options->backend->shutdown_func(options->backend); @@ -210,12 +227,17 @@ sentry_close(void) || !options->backend->can_capture_after_shutdown)) { sentry__run_clean(options->run); } - sentry_options_free(options); + } else { + SENTRY_DEBUG("sentry_close() called, but options was empty"); } + g_options = NULL; + sentry__mutex_unlock(&g_options_lock); + sentry__scope_cleanup(); sentry_clear_modulecache(); + return (int)dumped_envelopes; } @@ -343,7 +365,18 @@ sentry_capture_event(sentry_value_t event) was_captured = true; envelope = sentry__prepare_event(options, event, &event_id); if (envelope) { - sentry__add_current_session_to_envelope(envelope); + if (options->session) { + SENTRY_WITH_OPTIONS_MUT (mut_options) { + sentry__envelope_add_session( + envelope, mut_options->session); + // we're assuming that if a session is added to an envelope + // it will be sent onwards. This means we now need to set + // the init flag to false because we're no longer the + // initial session update. + mut_options->session->init = false; + } + } + sentry__capture_envelope(options->transport, envelope); } } @@ -460,10 +493,18 @@ sentry__ensure_event_id(sentry_value_t event, sentry_uuid_t *uuid_out) void sentry_set_user(sentry_value_t user) { + if (!sentry_value_is_null(user)) { + SENTRY_WITH_OPTIONS_MUT (options) { + if (options->session) { + sentry__session_sync_user(options->session, user); + sentry__run_write_session(options->run, options->session); + } + } + } + SENTRY_WITH_SCOPE_MUT (scope) { sentry_value_decref(scope->user); scope->user = user; - sentry__scope_session_sync(scope); } } diff --git a/src/sentry_core.h b/src/sentry_core.h index 46e546630..8040ef140 100644 --- a/src/sentry_core.h +++ b/src/sentry_core.h @@ -70,8 +70,22 @@ sentry_value_t sentry__ensure_event_id( */ const sentry_options_t *sentry__options_getref(void); +/** + * This will acquire a lock on the global options. + */ +sentry_options_t *sentry__options_lock(void); + +/** + * Release the lock on the global options. + */ +void sentry__options_unlock(void); + #define SENTRY_WITH_OPTIONS(Options) \ for (const sentry_options_t *Options = sentry__options_getref(); Options; \ sentry_options_free((sentry_options_t *)Options), Options = NULL) +#define SENTRY_WITH_OPTIONS_MUT(Options) \ + for (sentry_options_t *Options = sentry__options_lock(); Options; \ + sentry__options_unlock(), Options = NULL) + #endif diff --git a/src/sentry_logger.h b/src/sentry_logger.h index 51fd792f4..f17efbc6b 100644 --- a/src/sentry_logger.h +++ b/src/sentry_logger.h @@ -32,4 +32,6 @@ void sentry__logger_log(sentry_level_t level, const char *message, ...); #define SENTRY_WARN(message) sentry__logger_log(SENTRY_LEVEL_WARNING, message) +#define SENTRY_ERROR(message) sentry__logger_log(SENTRY_LEVEL_ERROR, message) + #endif diff --git a/src/sentry_options.h b/src/sentry_options.h index 09b727522..149a7fb06 100644 --- a/src/sentry_options.h +++ b/src/sentry_options.h @@ -4,6 +4,7 @@ #include "sentry_boot.h" #include "sentry_logger.h" +#include "sentry_session.h" #include "sentry_utils.h" // Defaults to 2s as per @@ -57,6 +58,7 @@ typedef struct sentry_options_s { /* everything from here on down are options which are stored here but not exposed through the options API */ struct sentry_backend_s *backend; + sentry_session_t *session; long user_consent; long refcount; diff --git a/src/sentry_scope.c b/src/sentry_scope.c index 143049e77..5f8992e7e 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -73,7 +73,6 @@ get_scope(void) g_scope.breadcrumbs = sentry_value_new_list(); g_scope.level = SENTRY_LEVEL_ERROR; g_scope.client_sdk = get_client_sdk(); - g_scope.session = NULL; g_scope_initialized = true; @@ -112,27 +111,16 @@ sentry__scope_unlock(void) } void -sentry__scope_flush_unlock(const sentry_scope_t *scope) +sentry__scope_flush_unlock() { - bool did_unlock = false; + sentry__scope_unlock(); SENTRY_WITH_OPTIONS (options) { - if (scope->session) { - sentry__run_write_session(options->run, scope->session); - sentry__scope_unlock(); - } else { - sentry__scope_unlock(); - sentry__run_clear_session(options->run); - } - did_unlock = true; // we try to unlock the scope/session lock as soon as possible. The // backend will do its own `WITH_SCOPE` internally. if (options->backend && options->backend->flush_scope_func) { options->backend->flush_scope_func(options->backend, options); } } - if (!did_unlock) { - sentry__scope_unlock(); - } } static void @@ -304,24 +292,3 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope, #undef IS_NULL #undef SET } - -void -sentry__scope_session_sync(sentry_scope_t *scope) -{ - if (!scope->session) { - return; - } - - if (!sentry_value_is_null(scope->user)) { - sentry_value_t did = sentry_value_get_by_key(scope->user, "id"); - if (sentry_value_is_null(did)) { - did = sentry_value_get_by_key(scope->user, "email"); - } - if (sentry_value_is_null(did)) { - did = sentry_value_get_by_key(scope->user, "username"); - } - sentry_value_decref(scope->session->distinct_id); - sentry_value_incref(did); - scope->session->distinct_id = did; - } -} diff --git a/src/sentry_scope.h b/src/sentry_scope.h index a6bb6155e..250ff8200 100644 --- a/src/sentry_scope.h +++ b/src/sentry_scope.h @@ -19,7 +19,6 @@ typedef struct sentry_scope_s { sentry_value_t breadcrumbs; sentry_level_t level; sentry_value_t client_sdk; - sentry_session_t *session; } sentry_scope_t; /** @@ -54,11 +53,11 @@ void sentry__scope_unlock(void); void sentry__scope_cleanup(void); /** - * This will notify any backend of scope changes, and persist session - * information to disk. This function must be called while holding the scope - * lock, and it will be unlocked internally. + * This will notify any backend of scope changes. + * This function must be called while holding the scope lock, and it will be + * unlocked internally. */ -void sentry__scope_flush_unlock(const sentry_scope_t *scope); +void sentry__scope_flush_unlock(); /** * This will merge the requested data which is in the given `scope` to the given @@ -70,12 +69,6 @@ void sentry__scope_apply_to_event(const sentry_scope_t *scope, const sentry_options_t *options, sentry_value_t event, sentry_scope_mode_t mode); -/** - * This will update a sessions `distinct_id`, which is generated out of other - * scope data. - */ -void sentry__scope_session_sync(sentry_scope_t *scope); - /** * These are convenience macros to automatically lock/unlock a scope inside a * code block. @@ -85,7 +78,7 @@ void sentry__scope_session_sync(sentry_scope_t *scope); sentry__scope_unlock(), Scope = NULL) #define SENTRY_WITH_SCOPE_MUT(Scope) \ for (sentry_scope_t *Scope = sentry__scope_lock(); Scope; \ - sentry__scope_flush_unlock(Scope), Scope = NULL) + sentry__scope_flush_unlock(), Scope = NULL) #define SENTRY_WITH_SCOPE_MUT_NO_FLUSH(Scope) \ for (sentry_scope_t *Scope = sentry__scope_lock(); Scope; \ sentry__scope_unlock(), Scope = NULL) diff --git a/src/sentry_session.c b/src/sentry_session.c index 38288a309..f58a88e88 100644 --- a/src/sentry_session.c +++ b/src/sentry_session.c @@ -1,5 +1,6 @@ #include "sentry_session.h" #include "sentry_alloc.h" +#include "sentry_database.h" #include "sentry_envelope.h" #include "sentry_json.h" #include "sentry_options.h" @@ -185,9 +186,10 @@ sentry__session_from_json(const char *buf, size_t buflen) sentry_value_get_by_key(value, "errors")); rv->started_ms = sentry__iso8601_to_msec( sentry_value_as_string(sentry_value_get_by_key(value, "started"))); - rv->duration_ms = (uint64_t)( - sentry_value_as_double(sentry_value_get_by_key(value, "duration")) - * 1000); + + double duration + = sentry_value_as_double(sentry_value_get_by_key(value, "duration")); + rv->duration_ms = (uint64_t)(duration * 1000); sentry_value_decref(value); @@ -212,18 +214,23 @@ void sentry_start_session(void) { sentry_end_session(); - SENTRY_WITH_SCOPE_MUT (scope) { - scope->session = sentry__session_new(); - sentry__scope_session_sync(scope); + SENTRY_WITH_SCOPE (scope) { + SENTRY_WITH_OPTIONS_MUT (options) { + options->session = sentry__session_new(); + if (options->session) { + sentry__session_sync_user(options->session, scope->user); + sentry__run_write_session(options->run, options->session); + } + } } } void sentry__record_errors_on_current_session(uint32_t error_count) { - SENTRY_WITH_SCOPE_MUT (scope) { - if (scope->session) { - scope->session->errors += error_count; + SENTRY_WITH_OPTIONS_MUT (options) { + if (options->session) { + options->session->errors += error_count; } } } @@ -232,9 +239,10 @@ static sentry_session_t * sentry__end_session_internal(void) { sentry_session_t *session = NULL; - SENTRY_WITH_SCOPE_MUT (scope) { - session = scope->session; - scope->session = NULL; + SENTRY_WITH_OPTIONS_MUT (options) { + session = options->session; + options->session = NULL; + sentry__run_clear_session(options->run); } if (session && session->status == SENTRY_SESSION_STATUS_OK) { @@ -271,15 +279,17 @@ sentry_end_session(void) } void -sentry__add_current_session_to_envelope(sentry_envelope_t *envelope) +sentry__session_sync_user(sentry_session_t *session, sentry_value_t user) { - SENTRY_WITH_SCOPE_MUT (scope) { - if (scope->session) { - sentry__envelope_add_session(envelope, scope->session); - // we're assuming that if a session is added to an envelope it - // will be sent onwards. This means we now need to set the init - // flag to false because we're no longer the initial session update. - scope->session->init = false; - } + + sentry_value_t did = sentry_value_get_by_key(user, "id"); + if (sentry_value_is_null(did)) { + did = sentry_value_get_by_key(user, "email"); + } + if (sentry_value_is_null(did)) { + did = sentry_value_get_by_key(user, "username"); } + sentry_value_decref(session->distinct_id); + sentry_value_incref(did); + session->distinct_id = did; } diff --git a/src/sentry_session.h b/src/sentry_session.h index 6de5f7ec1..5ba036cbd 100644 --- a/src/sentry_session.h +++ b/src/sentry_session.h @@ -28,7 +28,7 @@ typedef struct sentry_session_s { uint64_t duration_ms; uint64_t errors; sentry_session_status_t status; - bool init; + long init; } sentry_session_t; /** @@ -71,8 +71,8 @@ sentry_session_t *sentry__end_current_session_with_status( void sentry__record_errors_on_current_session(uint32_t error_count); /** - * Add the current session an a new envelope item to `envelope`. + * This will update a sessions `distinct_id`, which is based on the user. */ -void sentry__add_current_session_to_envelope(sentry_envelope_t *envelope); +void sentry__session_sync_user(sentry_session_t *session, sentry_value_t user); #endif diff --git a/src/sentry_sync.c b/src/sentry_sync.c index d81b21628..3b8746beb 100644 --- a/src/sentry_sync.c +++ b/src/sentry_sync.c @@ -207,24 +207,7 @@ sentry__bgworker_is_done(sentry_bgworker_t *bgw) return !bgw->first_task && !sentry__atomic_fetch(&bgw->running); } -#ifdef _MSC_VER -# define THREAD_FUNCTION_API __stdcall -#else -# define THREAD_FUNCTION_API -#endif - -#if defined(__MINGW32__) && !defined(__MINGW64__) -# define UNSIGNED_MINGW unsigned -#else -# define UNSIGNED_MINGW -#endif - -// pthreads use `void *` return types, whereas windows uses `DWORD` -#ifdef SENTRY_PLATFORM_WINDOWS -static UNSIGNED_MINGW DWORD THREAD_FUNCTION_API -#else -static void * -#endif +SENTRY_THREAD_FN worker_thread(void *data) { sentry_bgworker_t *bgw = data; diff --git a/src/sentry_sync.h b/src/sentry_sync.h index d7586c8dd..5c3cbe3ef 100644 --- a/src/sentry_sync.h +++ b/src/sentry_sync.h @@ -7,6 +7,25 @@ #include #include +#ifdef _MSC_VER +# define THREAD_FUNCTION_API __stdcall +#else +# define THREAD_FUNCTION_API +#endif + +#if defined(__MINGW32__) && !defined(__MINGW64__) +# define UNSIGNED_MINGW unsigned +#else +# define UNSIGNED_MINGW +#endif + +// pthreads use `void *` return types, whereas windows uses `DWORD` +#ifdef SENTRY_PLATFORM_WINDOWS +# define SENTRY_THREAD_FN static UNSIGNED_MINGW DWORD THREAD_FUNCTION_API +#else +# define SENTRY_THREAD_FN static void * +#endif + // define a recursive mutex for all platforms #ifdef SENTRY_PLATFORM_WINDOWS # if _WIN32_WINNT >= 0x0600 @@ -200,10 +219,10 @@ typedef CONDITION_VARIABLE sentry_cond_t; we're restricted in what we can do. In particular it's possible that we would end up dead locking ourselves. While we cannot fully prevent races we have a logic here that while the signal handler is active we're - disabling our mutexes so that our signal handler can access what otherwise - would be protected by the mutex but everyone else needs to wait for the - signal handler to finish. This is not without risk because another thread - might still access what the mutex protects. + disabling our mutexes so that our signal handler can access what + otherwise would be protected by the mutex but everyone else needs to wait + for the signal handler to finish. This is not without risk because + another thread might still access what the mutex protects. We are thus taking care that whatever such mutexes protect will not make us crash under concurrent modifications. The mutexes we're likely going diff --git a/tests/cmake.py b/tests/cmake.py index c81c3cc4f..b9b8860bd 100644 --- a/tests/cmake.py +++ b/tests/cmake.py @@ -129,6 +129,8 @@ def cmake(cwd, targets, options=None): configcmd.append("-DSENTRY_BUILD_FORCE32=ON") if "asan" in os.environ.get("RUN_ANALYZER", ""): configcmd.append("-DWITH_ASAN_OPTION=ON") + if "tsan" in os.environ.get("RUN_ANALYZER", ""): + configcmd.append("-DWITH_TSAN_OPTION=ON") # we have to set `-Werror` for this cmake invocation only, otherwise # completely unrelated things will break diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index adecacdfa..91a7e9faf 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -20,6 +20,7 @@ add_executable(sentry_test_unit test_attachments.c test_basic.c test_consent.c + test_concurrency.c test_envelopes.c test_failures.c test_fuzzfailures.c diff --git a/tests/unit/test_concurrency.c b/tests/unit/test_concurrency.c new file mode 100644 index 000000000..e2c1446bf --- /dev/null +++ b/tests/unit/test_concurrency.c @@ -0,0 +1,97 @@ +#include "sentry_core.h" +#include "sentry_testsupport.h" +#include +#include + +static void +send_envelope_test_concurrent(const sentry_envelope_t *envelope, void *data) +{ + sentry__atomic_fetch_and_add((long *)data, 1); + + sentry_value_t event = sentry_envelope_get_event(envelope); + if (!sentry_value_is_null(event)) { + const char *event_id = sentry_value_as_string( + sentry_value_get_by_key(event, "event_id")); + TEST_CHECK_STRING_EQUAL( + event_id, "4c035723-8638-4c3a-923f-2ab9d08b4018"); + } +} + +static void +init_framework(long *called) +{ + sentry_options_t *options = sentry_options_new(); + sentry_options_set_dsn(options, "https://foo@sentry.invalid/42"); + sentry_options_set_transport(options, + sentry_new_function_transport(send_envelope_test_concurrent, called)); + sentry_options_set_release(options, "prod"); + sentry_options_set_require_user_consent(options, false); + sentry_options_set_auto_session_tracking(options, true); + sentry_init(options); +} + +SENTRY_TEST(multiple_inits) +{ + long called = 0; + + init_framework(&called); + init_framework(&called); + + sentry_set_transaction("demo-trans"); + + sentry_capture_event(sentry_value_new_message_event( + SENTRY_LEVEL_INFO, "root", "Hello World!")); + + sentry_value_t obj = sentry_value_new_object(); + // something that is not a uuid, as this will be forcibly changed + sentry_value_set_by_key(obj, "event_id", sentry_value_new_int32(1234)); + sentry_capture_event(obj); + + sentry_close(); + sentry_close(); + + TEST_CHECK_INT_EQUAL(called, 4); +} + +SENTRY_THREAD_FN +thread_worker(void *called) +{ + init_framework(called); + + sentry_set_transaction("demo-trans"); + + sentry_capture_event(sentry_value_new_message_event( + SENTRY_LEVEL_INFO, "root", "Hello World!")); + + sentry_value_t obj = sentry_value_new_object(); + // something that is not a uuid, as this will be forcibly changed + sentry_value_set_by_key(obj, "event_id", sentry_value_new_int32(1234)); + sentry_capture_event(obj); + + return 0; +} + +SENTRY_TEST(concurrent_init) +{ + long called = 0; + +#define THREADS_NUM 10 + sentry_threadid_t threads[THREADS_NUM]; + + for (size_t i = 0; i < THREADS_NUM; i++) { + sentry__thread_init(&threads[i]); + sentry__thread_spawn(&threads[i], &thread_worker, &called); + } + for (size_t i = 0; i < THREADS_NUM; i++) { + sentry__thread_join(threads[i]); + sentry__thread_free(&threads[i]); + } + + sentry_close(); + + // 1 session, and up to 2 events per thread. + // might be less because `capture_event` races with close/init and might + // lose events. + TEST_CHECK(called >= THREADS_NUM * 1); + TEST_CHECK(called <= THREADS_NUM * 3); +} diff --git a/tests/unit/test_session.c b/tests/unit/test_session.c index e79cc266b..5864526f1 100644 --- a/tests/unit/test_session.c +++ b/tests/unit/test_session.c @@ -93,9 +93,12 @@ send_sampled_envelope(const sentry_envelope_t *envelope, void *data) { session_assertion_t *assertion = data; + SENTRY_DEBUG("send_sampled_envelope"); if (assertion->assert_session) { assertion->called += 1; + SENTRY_DEBUG("assertion + 1"); + TEST_CHECK_INT_EQUAL(sentry__envelope_get_item_count(envelope), 1); const sentry_envelope_item_t *item diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index 08908530a..eb1626160 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -5,6 +5,7 @@ XX(basic_http_request_preparation_for_event) XX(basic_http_request_preparation_for_event_with_attachment) XX(basic_http_request_preparation_for_minidump) XX(buildid_fallback) +XX(concurrent_init) XX(count_sampled_events) XX(custom_logger) XX(dsn_parsing_complete) @@ -22,6 +23,7 @@ XX(module_addr) XX(module_finder) XX(mpack_newlines) XX(mpack_removed_tags) +XX(multiple_inits) XX(os) XX(page_allocator) XX(path_basics)