fix: Lock scope for duration of flush hook (#348)

Also add some tests and fix winhttp proxy validation.

CHANGELOG.md

@@ -12,6 +12,7 @@
 - The return value of the transport shutdown hook set via `sentry_transport_set_shutdown_func` was also changed to return an `int`.
 - Both functions should return _0_ on success, and a non-zero error code on failure, as does `sentry_init`.
 - Similarly, the return value of `sentry_shutdown` was also changed to an `int`, and will return _0_ on success and a non-zero error code on unclean shutdown.
+- Documentation for custom transports was updated to highlight the ordering requirements of submitted envelopes, which is important for release health.
 
 ```c
 // before

include/sentry.h

@@ -703,6 +703,8 @@ SENTRY_API const char *sentry_options_get_dist(const sentry_options_t *opts);
 
 /**
  * Configures the http proxy.
+ *
+ * The given proxy has to include the full scheme, eg. `http://some.proxy/`.
  */
 SENTRY_API void sentry_options_set_http_proxy(
     sentry_options_t *opts, const char *proxy);

src/backends/sentry_backend_crashpad.cpp

@@ -65,8 +65,7 @@ sentry__crashpad_backend_user_consent_changed(sentry_backend_t *backend)
 }
 
 static void
-sentry__crashpad_backend_flush_scope(
-    sentry_backend_t *backend, const sentry_scope_t *UNUSED(scope))
+sentry__crashpad_backend_flush_scope(sentry_backend_t *backend)
 {
     const crashpad_state_t *data = (crashpad_state_t *)backend->data;
     if (!data->event_path) {

src/sentry_backend.h

@@ -19,8 +19,7 @@ typedef struct sentry_backend_s {
     void (*free_func)(struct sentry_backend_s *);
     void (*except_func)(
         struct sentry_backend_s *, const struct sentry_ucontext_s *);
-    void (*flush_scope_func)(
-        struct sentry_backend_s *, const sentry_scope_t *scope);
+    void (*flush_scope_func)(struct sentry_backend_s *);
     void (*add_breadcrumb_func)(
         struct sentry_backend_s *, sentry_value_t breadcrumb);
     void (*user_consent_changed_func)(struct sentry_backend_s *);

src/sentry_core.c

@@ -108,7 +108,8 @@ sentry_init(sentry_options_t *options)
 
     if (!options->dsn || !options->dsn->is_valid) {
         const char *raw_dsn = sentry_options_get_dsn(options);
-        SENTRY_WARNF("the provided DSN \"%s\" is not valid", raw_dsn || "");
+        SENTRY_WARNF(
+            "the provided DSN \"%s\" is not valid", raw_dsn ? raw_dsn : "");
     }
 
     if (transport) {

src/sentry_scope.c

@@ -96,17 +96,26 @@ sentry__scope_unlock(void)
 }
 
 void
-sentry__scope_flush(const sentry_scope_t *scope)
+sentry__scope_flush_unlock(const sentry_scope_t *scope)
 {
+    bool did_unlock = false;
     SENTRY_WITH_OPTIONS (options) {
-        if (options->backend && options->backend->flush_scope_func) {
-            options->backend->flush_scope_func(options->backend, scope);
-        }
         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);
+        }
+    }
+    if (!did_unlock) {
+        sentry__scope_unlock();
     }
 }
 

src/sentry_scope.h

@@ -55,9 +55,10 @@ void sentry__scope_cleanup(void);
 
 /**
  * This will notify any backend of scope changes, and persist session
- * information to disk.
+ * information to disk. This function must be called while holding the scope
+ * lock, and it will be unlocked internally.
  */
-void sentry__scope_flush(const sentry_scope_t *scope);
+void sentry__scope_flush_unlock(const sentry_scope_t *scope);
 
 /**
  * This will merge the requested data which is in the given `scope` to the given
@@ -83,7 +84,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_unlock(), sentry__scope_flush(scope), Scope = NULL)
+         sentry__scope_flush_unlock(Scope), Scope = NULL)
 #define SENTRY_WITH_SCOPE_MUT_NO_FLUSH(Scope)                                  \
     for (sentry_scope_t *Scope = sentry__scope_lock(); Scope;                  \
          sentry__scope_unlock(), Scope = NULL)

src/transports/sentry_transport_winhttp.c

@@ -65,7 +65,9 @@ sentry__winhttp_transport_start(
     state->user_agent = sentry__string_to_wstr(SENTRY_SDK_USER_AGENT);
     state->debug = opts->debug;
 
-    if (opts->http_proxy && strstr(opts->http_proxy, "http://") == 0) {
+    // ensure the proxy starts with `http://`, otherwise ignore it
+    if (opts->http_proxy
+        && strstr(opts->http_proxy, "http://") == opts->http_proxy) {
         const char *ptr = opts->http_proxy + 7;
         const char *slash = strchr(ptr, '/');
         if (slash) {

tests/test_unit.py

@@ -1,5 +1,7 @@
 import os
+import pytest
 from . import run
+from .conditions import has_http
 
 
 def test_unit(cmake, unittest):
@@ -8,3 +10,10 @@ def test_unit(cmake, unittest):
     )
     env = dict(os.environ)
     run(cwd, "sentry_test_unit", ["--no-summary", unittest], check=True, env=env)
+
+
+@pytest.mark.skipif(not has_http, reason="tests need http transport")
+def test_unit_transport(cmake, unittest):
+    cwd = cmake(["sentry_test_unit"], {"SENTRY_BACKEND": "none"})
+    env = dict(os.environ)
+    run(cwd, "sentry_test_unit", ["--no-summary", unittest], check=True, env=env)

tests/unit/test_uninit.c

@@ -31,3 +31,48 @@ SENTRY_TEST(uninitialized)
     sentry_end_session();
     sentry_shutdown();
 }
+
+SENTRY_TEST(empty_transport)
+{
+    sentry_options_t *options = sentry_options_new();
+    sentry_options_set_transport(options, NULL);
+
+    TEST_CHECK(sentry_init(options) == 0);
+
+    sentry_value_t event = sentry_value_new_message_event(
+        SENTRY_LEVEL_WARNING, NULL, "some message");
+    sentry_uuid_t id = sentry_capture_event(event);
+    TEST_CHECK(!sentry_uuid_is_nil(&id));
+
+    sentry_shutdown();
+}
+
+SENTRY_TEST(invalid_dsn)
+{
+    sentry_options_t *options = sentry_options_new();
+    sentry_options_set_dsn(options, "not a valid dsn");
+
+    TEST_CHECK(sentry_init(options) == 0);
+
+    sentry_value_t event = sentry_value_new_message_event(
+        SENTRY_LEVEL_WARNING, NULL, "some message");
+    sentry_uuid_t id = sentry_capture_event(event);
+    TEST_CHECK(!sentry_uuid_is_nil(&id));
+
+    sentry_shutdown();
+}
+
+SENTRY_TEST(invalid_proxy)
+{
+    sentry_options_t *options = sentry_options_new();
+    sentry_options_set_http_proxy(options, "invalid");
+
+    TEST_CHECK(sentry_init(options) == 0);
+
+    sentry_value_t event = sentry_value_new_message_event(
+        SENTRY_LEVEL_WARNING, NULL, "some message");
+    sentry_uuid_t id = sentry_capture_event(event);
+    TEST_CHECK(!sentry_uuid_is_nil(&id));
+
+    sentry_shutdown();
+}

tests/unit/tests.inc

@@ -10,7 +10,10 @@ XX(dsn_parsing_complete)
 XX(dsn_parsing_invalid)
 XX(dsn_store_url_with_path)
 XX(dsn_store_url_without_path)
+XX(empty_transport)
 XX(init_failure)
+XX(invalid_dsn)
+XX(invalid_proxy)
 XX(iso_time)
 XX(lazy_attachments)
 XX(module_finder)