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: deadlock pattern in dynamic sdk-name assignment #857

Closed
wants to merge 3 commits into from
Closed
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

**Fixes**:

- Remove deadlock pattern in dynamic sdk-name assignment ([#857](https://github.com/getsentry/sentry-native/pull/857))

## 0.6.4

**Fixes**:
Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ The example currently supports the following commands:
- `discarding-before-send`: Installs a `before_send()` callback that discards the event.
- `on-crash`: Installs an `on_crash()` callback that retains the crash event.
- `discarding-on-crash`: Installs an `on_crash()` callback that discards the crash event.
- `override-sdk-name`: Changes the SDK name via the options at runtime.

Only on Windows using crashpad with its WER handler module:

Expand Down
4 changes: 4 additions & 0 deletions examples/example.c
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,10 @@ main(int argc, char **argv)
options, discarding_on_crash_callback, NULL);
}

if (has_arg(argc, argv, "override-sdk-name")) {
sentry_options_set_sdk_name(options, "sentry.native.android.flutter");
}

sentry_init(options);

if (!has_arg(argc, argv, "no-setup")) {
Expand Down
4 changes: 1 addition & 3 deletions src/sentry_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,7 @@ sentry_init(sentry_options_t *options)
// since at least crashpad needs that.
// the only way to get a reference to the scope is by locking it, the macro
// does all that at once, including invoking the backends scope flush hook
SENTRY_WITH_SCOPE_MUT (scope) {
(void)scope;
}
SENTRY_WITH_SCOPE_MUT_AND_OPTIONS(scope, options) { (void)scope; }
if (backend && backend->user_consent_changed_func) {
backend->user_consent_changed_func(backend);
}
Expand Down
17 changes: 12 additions & 5 deletions src/sentry_scope.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ static sentry_scope_t g_scope = { 0 };
static sentry_mutex_t g_lock = SENTRY__MUTEX_INIT;

static sentry_value_t
get_client_sdk(void)
get_client_sdk(const sentry_options_t *options)
{
sentry_value_t client_sdk = sentry_value_new_object();

SENTRY_WITH_OPTIONS (options) {
if (options) {
sentry_value_t sdk_name = sentry_value_new_string(options->sdk_name);
sentry_value_set_by_key(client_sdk, "name", sdk_name);
}
Expand Down Expand Up @@ -66,7 +66,7 @@ get_client_sdk(void)
}

static sentry_scope_t *
get_scope(void)
get_scope(const sentry_options_t *options)
{
if (g_scope_initialized) {
return &g_scope;
Expand All @@ -82,7 +82,7 @@ get_scope(void)
sentry_value_set_by_key(g_scope.contexts, "os", sentry__get_os_context());
g_scope.breadcrumbs = sentry_value_new_list();
g_scope.level = SENTRY_LEVEL_ERROR;
g_scope.client_sdk = get_client_sdk();
g_scope.client_sdk = get_client_sdk(options);
g_scope.transaction_object = NULL;
g_scope.span = NULL;

Expand Down Expand Up @@ -115,7 +115,14 @@ sentry_scope_t *
sentry__scope_lock(void)
{
sentry__mutex_lock(&g_lock);
return get_scope();
return get_scope(NULL);
}

sentry_scope_t *
sentry__scope_lock_with_options(const sentry_options_t *options)
{
sentry__mutex_lock(&g_lock);
return get_scope(options);
}

void
Expand Down
6 changes: 6 additions & 0 deletions src/sentry_scope.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ sentry_scope_t *sentry__scope_lock(void);
*/
void sentry__scope_unlock(void);

sentry_scope_t *sentry__scope_lock_with_options(
const sentry_options_t *options);

/**
* This will free all the data attached to the global scope
*/
Expand Down Expand Up @@ -93,6 +96,9 @@ void sentry__scope_apply_to_event(const sentry_scope_t *scope,
#define SENTRY_WITH_SCOPE_MUT_NO_FLUSH(Scope) \
for (sentry_scope_t *Scope = sentry__scope_lock(); Scope; \
sentry__scope_unlock(), Scope = NULL)
#define SENTRY_WITH_SCOPE_MUT_AND_OPTIONS(Scope, Options) \
for (sentry_scope_t *Scope = sentry__scope_lock_with_options(Options); \
Scope; sentry__scope_flush_unlock(), Scope = NULL)

#endif

Expand Down
21 changes: 20 additions & 1 deletion tests/test_integration_stdout.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
assert_event,
assert_crash,
assert_minidump,
assert_timestamp,
assert_before_send,
assert_no_before_send,
assert_crash_timestamp,
Expand Down Expand Up @@ -46,6 +45,26 @@ def test_capture_stdout(cmake):
assert_event(envelope)


def test_dynamic_sdk_name_override(cmake):
tmp_path = cmake(
["sentry_example"],
{
"SENTRY_BACKEND": "none",
"SENTRY_TRANSPORT": "none",
},
)

output = check_output(
tmp_path,
"sentry_example",
["stdout", "override-sdk-name", "capture-event"],
)
envelope = Envelope.deserialize(output)

assert_meta(envelope, sdk_override="sentry.native.android.flutter")
assert_event(envelope)


def test_sdk_name_override(cmake):
sdk_name = "cUsToM.SDK"
tmp_path = cmake(
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/test_concurrency.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ SENTRY_TEST(multiple_inits)
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
// 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);

Expand All @@ -64,7 +64,7 @@ thread_worker(void *called)
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
// 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);

Expand All @@ -75,7 +75,7 @@ SENTRY_TEST(concurrent_init)
{
long called = 0;

#define THREADS_NUM 10
#define THREADS_NUM 100
sentry_threadid_t threads[THREADS_NUM];

for (size_t i = 0; i < THREADS_NUM; i++) {
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test_options.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ SENTRY_TEST(options_sdk_name_invalid)
const char *sdk_name = NULL;
const int result = sentry_options_set_sdk_name(options, sdk_name);

// then the value should should be ignored
// then the value should be ignored
TEST_CHECK_INT_EQUAL(result, 1);
TEST_CHECK_STRING_EQUAL(
sentry_options_get_sdk_name(options), SENTRY_SDK_NAME);
Expand Down