Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Avoid deadlocks with uninitialized options #639

Merged
merged 1 commit into from
Dec 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions src/sentry_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -372,15 +372,14 @@ sentry_capture_event(sentry_value_t event)
envelope = sentry__prepare_event(options, event, &event_id);
if (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_options_t *mut_options = sentry__options_lock();
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__options_unlock();
}

sentry__capture_envelope(options->transport, envelope);
Expand Down Expand Up @@ -535,12 +534,12 @@ 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_options_t *options = sentry__options_lock();
if (options && options->session) {
sentry__session_sync_user(options->session, user);
sentry__run_write_session(options->run, options->session);
}
sentry__options_unlock();
}

SENTRY_WITH_SCOPE_MUT (scope) {
Expand Down
4 changes: 0 additions & 4 deletions src/sentry_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,6 @@ void sentry__options_unlock(void);
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)

// these for now are only needed for tests
#ifdef SENTRY_UNITTEST
bool sentry__roll_dice(double probability);
Expand Down
16 changes: 10 additions & 6 deletions src/sentry_session.c
Original file line number Diff line number Diff line change
Expand Up @@ -215,35 +215,39 @@ sentry_start_session(void)
{
sentry_end_session();
SENTRY_WITH_SCOPE (scope) {
SENTRY_WITH_OPTIONS_MUT (options) {
sentry_options_t *options = sentry__options_lock();
if (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);
}
}
sentry__options_unlock();
}
}

void
sentry__record_errors_on_current_session(uint32_t error_count)
{
SENTRY_WITH_OPTIONS_MUT (options) {
if (options->session) {
options->session->errors += error_count;
}
sentry_options_t *options = sentry__options_lock();
if (options && options->session) {
options->session->errors += error_count;
}
sentry__options_unlock();
}

static sentry_session_t *
sentry__end_session_internal(void)
{
sentry_session_t *session = NULL;
SENTRY_WITH_OPTIONS_MUT (options) {
sentry_options_t *options = sentry__options_lock();
if (options) {
session = options->session;
options->session = NULL;
sentry__run_clear_session(options->run);
}
sentry__options_unlock();

if (session && session->status == SENTRY_SESSION_STATUS_OK) {
session->status = SENTRY_SESSION_STATUS_EXITED;
Expand Down
27 changes: 27 additions & 0 deletions tests/unit/test_concurrency.c
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,30 @@ SENTRY_TEST(concurrent_init)
TEST_CHECK(called >= THREADS_NUM * 1);
TEST_CHECK(called <= THREADS_NUM * 3);
}

SENTRY_THREAD_FN
thread_breadcrumb(void *UNUSED(arg))
{
sentry_value_t breadcrumb = sentry_value_new_breadcrumb("foo", "bar");
sentry_add_breadcrumb(breadcrumb);

return 0;
}

SENTRY_TEST(concurrent_uninit)
{
sentry_value_t user = sentry_value_new_object();
sentry_set_user(user);

sentry_threadid_t thread;
sentry__thread_init(&thread);
sentry__thread_spawn(&thread, &thread_breadcrumb, NULL);

sentry_value_t breadcrumb = sentry_value_new_breadcrumb("foo", "bar");
sentry_add_breadcrumb(breadcrumb);

sentry__thread_join(thread);
sentry__thread_free(&thread);

sentry_close();
}
1 change: 1 addition & 0 deletions tests/unit/tests.inc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ XX(basic_tracing_context)
XX(basic_transaction)
XX(buildid_fallback)
XX(concurrent_init)
XX(concurrent_uninit)
XX(count_sampled_events)
XX(custom_logger)
XX(dsn_parsing_complete)
Expand Down