From 70f84afed4faf7a3cda68aae0f741d6cf43b6f0b Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Thu, 20 Feb 2025 14:03:23 +0100 Subject: [PATCH 01/14] update crashpad --- external/crashpad | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/crashpad b/external/crashpad index 8a447fc9d..3893f32ce 160000 --- a/external/crashpad +++ b/external/crashpad @@ -1 +1 @@ -Subproject commit 8a447fc9d0ca921dd5654c3ce18779f455c218b1 +Subproject commit 3893f32ce1f414fd3a7c8675bbdd5d8a7594f8c1 From 3f4df97e65145b46dded70b2edc3e4946c17f1bc Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Thu, 20 Feb 2025 14:11:13 +0100 Subject: [PATCH 02/14] add sentry option `on_crash_wait_for_upload` --- include/sentry.h | 17 +++++++++++++---- src/sentry_options.c | 8 ++++++++ src/sentry_options.h | 1 + 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/include/sentry.h b/include/sentry.h index feabc47a6..fb68843fe 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -1272,11 +1272,20 @@ SENTRY_API void sentry_options_set_system_crash_reporter_enabled( sentry_options_t *opts, int enabled); /** - * Sets the maximum time (in milliseconds) to wait for the asynchronous tasks to - * end on shutdown, before attempting a forced termination. + * Enables a wait for the crash report upload to be finished before shutting + * down. This is disabled by default. + * + * This setting only has an effect when using the crashpad backend. */ -SENTRY_API void sentry_options_set_shutdown_timeout( - sentry_options_t *opts, uint64_t shutdown_timeout); +SENTRY_API void sentry_options_set_on_crash_wait_for_upload( + sentry_options_t *opts, int wait_for_upload) + + /** + * Sets the maximum time (in milliseconds) to wait for the asynchronous + * tasks to end on shutdown, before attempting a forced termination. + */ + SENTRY_API void sentry_options_set_shutdown_timeout( + sentry_options_t *opts, uint64_t shutdown_timeout); /** * Gets the maximum time (in milliseconds) to wait for the asynchronous tasks to diff --git a/src/sentry_options.c b/src/sentry_options.c index b4fdea8a6..9d6fa3f64 100644 --- a/src/sentry_options.c +++ b/src/sentry_options.c @@ -47,6 +47,7 @@ sentry_options_new(void) opts->user_consent = SENTRY_USER_CONSENT_UNKNOWN; opts->auto_session_tracking = true; opts->system_crash_reporter_enabled = false; + opts->on_crash_wait_for_upload = false; opts->symbolize_stacktraces = // AIX doesn't have reliable debug IDs for server-side symbolication, // and the diversity of Android makes it infeasible to have access to debug @@ -451,6 +452,13 @@ sentry_options_set_system_crash_reporter_enabled( opts->system_crash_reporter_enabled = !!enabled; } +void +sentry_options_set_on_crash_wait_for_upload( + sentry_options_t *opts, int wait_for_upload) +{ + opts->on_crash_wait_for_upload = !!wait_for_upload; +} + void sentry_options_set_shutdown_timeout( sentry_options_t *opts, uint64_t shutdown_timeout) diff --git a/src/sentry_options.h b/src/sentry_options.h index 521e9e5e1..c472f20d0 100644 --- a/src/sentry_options.h +++ b/src/sentry_options.h @@ -49,6 +49,7 @@ typedef struct sentry_options_s { bool require_user_consent; bool symbolize_stacktraces; bool system_crash_reporter_enabled; + bool on_crash_wait_for_upload; sentry_attachment_t *attachments; sentry_run_t *run; From 86618a4282e1f566691e394ad05774859a41f3c2 Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Thu, 20 Feb 2025 14:14:24 +0100 Subject: [PATCH 03/14] add CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e088a23e4..c0fd15d0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ **Features**: - Ensure support for `http_proxy` and `https_proxy` environment variables across all transports. ([#1111](https://github.com/getsentry/sentry-native/pull/1111)) +- Add option for `Crashpad` to delay shutdown until upload thread completes ([#1153](https://github.com/getsentry/sentry-native/pull/1153)) **Fixes**: From 1e80d36810ae1d0716223eec6cf108bf94eff195 Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Thu, 20 Feb 2025 14:22:32 +0100 Subject: [PATCH 04/14] typo --- include/sentry.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/include/sentry.h b/include/sentry.h index fb68843fe..d14d445d1 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -1278,14 +1278,14 @@ SENTRY_API void sentry_options_set_system_crash_reporter_enabled( * This setting only has an effect when using the crashpad backend. */ SENTRY_API void sentry_options_set_on_crash_wait_for_upload( - sentry_options_t *opts, int wait_for_upload) - - /** - * Sets the maximum time (in milliseconds) to wait for the asynchronous - * tasks to end on shutdown, before attempting a forced termination. - */ - SENTRY_API void sentry_options_set_shutdown_timeout( - sentry_options_t *opts, uint64_t shutdown_timeout); + sentry_options_t *opts, int wait_for_upload); + +/** + * Sets the maximum time (in milliseconds) to wait for the asynchronous + * tasks to end on shutdown, before attempting a forced termination. + */ +SENTRY_API void sentry_options_set_shutdown_timeout( + sentry_options_t *opts, uint64_t shutdown_timeout); /** * Gets the maximum time (in milliseconds) to wait for the asynchronous tasks to From bc6a28c23bf6c13859d5586c83f9b63e8f0c4e6b Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Mon, 24 Feb 2025 11:58:29 +0100 Subject: [PATCH 05/14] update crashpad + pass option --- external/crashpad | 2 +- src/backends/sentry_backend_crashpad.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/external/crashpad b/external/crashpad index 3893f32ce..843f50171 160000 --- a/external/crashpad +++ b/external/crashpad @@ -1 +1 @@ -Subproject commit 3893f32ce1f414fd3a7c8675bbdd5d8a7594f8c1 +Subproject commit 843f50171b54f3dbfc34b56299f25c8554a5c8b0 diff --git a/src/backends/sentry_backend_crashpad.cpp b/src/backends/sentry_backend_crashpad.cpp index 85de1bf71..342bfa67c 100644 --- a/src/backends/sentry_backend_crashpad.cpp +++ b/src/backends/sentry_backend_crashpad.cpp @@ -454,7 +454,8 @@ crashpad_backend_startup( bool success = data->client->StartHandler(handler, database, database, minidump_url ? minidump_url : "", proxy_url, annotations, arguments, /* restartable */ true, - /* asynchronous_start */ false, attachments); + /* asynchronous_start */ false, attachments, + options->on_crash_wait_for_upload); sentry_free(minidump_url); #ifdef SENTRY_PLATFORM_WINDOWS From bd5ecfed72233a7be04c004d8bd19d3731ac4faa Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Mon, 24 Feb 2025 12:17:22 +0100 Subject: [PATCH 06/14] update crashpad --- external/crashpad | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/crashpad b/external/crashpad index 843f50171..b916ce06d 160000 --- a/external/crashpad +++ b/external/crashpad @@ -1 +1 @@ -Subproject commit 843f50171b54f3dbfc34b56299f25c8554a5c8b0 +Subproject commit b916ce06deca0abae3093b0877c048c4570cddcc From 40a706c70fd4c473a515ae3a1c281a9855777aeb Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Mon, 24 Feb 2025 13:20:51 +0100 Subject: [PATCH 07/14] add `crashpad-wait-for-upload` to example --- CONTRIBUTING.md | 4 ++++ examples/example.c | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c9bf5ac8b..b51c8fce6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -157,6 +157,10 @@ The example currently supports the following commands: - `capture-transaction`: Captures a transaction. - `traces-sampler`: Installs a traces sampler callback function when used alongside `capture-transaction`. + +Only on Linux using crashpad: +- `crashpad-wait-for-upload`: Couples application shutdown to finishing the upload thread. + Only on Windows using crashpad with its WER handler module: - `fastfail`: Crashes the application using the `__fastfail` intrinsic directly, thus by-passing SEH. diff --git a/examples/example.c b/examples/example.c index 6ac07fde6..0078ba511 100644 --- a/examples/example.c +++ b/examples/example.c @@ -286,6 +286,10 @@ main(int argc, char **argv) sentry_options_set_proxy(options, "socks5://127.0.0.1:1080"); } + if (has_arg(argc, argv, "crashpad-wait-for-upload")) { + sentry_options_set_on_crash_wait_for_upload(options, true); + } + sentry_init(options); if (!has_arg(argc, argv, "no-setup")) { From 7dfb0737a851eefc2f9600869b6b7119f258347a Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Tue, 25 Feb 2025 11:06:54 +0100 Subject: [PATCH 08/14] update crashpad + add empty test --- external/crashpad | 2 +- tests/test_integration_crashpad.py | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/external/crashpad b/external/crashpad index b916ce06d..b91b6545b 160000 --- a/external/crashpad +++ b/external/crashpad @@ -1 +1 @@ -Subproject commit b916ce06deca0abae3093b0877c048c4570cddcc +Subproject commit b91b6545b1dcbbab15f25dca6e8b767facc07832 diff --git a/tests/test_integration_crashpad.py b/tests/test_integration_crashpad.py index 4795c3aa2..cbb121e48 100644 --- a/tests/test_integration_crashpad.py +++ b/tests/test_integration_crashpad.py @@ -519,3 +519,30 @@ def test_disable_backend(cmake, httpserver): # crashpad is disabled, and we are only crashing, so we expect no requests assert len(httpserver.log) == 0 + + +@pytest.mark.skipif( + sys.platform != "linux", + reason="on_crash_wait_for_upload option is only honored on linux", +) +def test_wait_for_upload(cmake, httpserver): + pass # pass for now + # tmp_path = cmake(["sentry_example"], {"SENTRY_BACKEND": "crashpad"}) + # + # # make sure we are isolated from previous runs + # shutil.rmtree(tmp_path / ".sentry-native", ignore_errors=True) + # + # env = dict(os.environ, SENTRY_DSN=make_dsn(httpserver)) + # httpserver.expect_oneshot_request("/api/123456/minidump/").respond_with_data("OK") + # + # # TODO figure out a way to kill the example, but keep crashpad alive + # with httpserver.wait(timeout=10) as waiting: + # child = run( + # tmp_path, "sentry_example", ["log", "crashpad-wait-for-upload", "crash"], env=env + # ) + # assert child.returncode # well, it's a crash after all + # + # assert waiting.result + # + # + # assert len(httpserver.log) == 1 From 75e22044148639a4a2ccd243240141ccf79775be Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Thu, 6 Mar 2025 15:12:43 +0100 Subject: [PATCH 09/14] update crashpad --- external/crashpad | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/crashpad b/external/crashpad index b91b6545b..a0f86ad5a 160000 --- a/external/crashpad +++ b/external/crashpad @@ -1 +1 @@ -Subproject commit b91b6545b1dcbbab15f25dca6e8b767facc07832 +Subproject commit a0f86ad5a71b925864d99d07821bae655e8a4a73 From 35a6ea6fc3b288bbece59b0ef68501170195b805 Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Mon, 24 Mar 2025 16:49:18 +0100 Subject: [PATCH 10/14] update crashpad --- external/crashpad | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/crashpad b/external/crashpad index a0f86ad5a..80d72e756 160000 --- a/external/crashpad +++ b/external/crashpad @@ -1 +1 @@ -Subproject commit a0f86ad5a71b925864d99d07821bae655e8a4a73 +Subproject commit 80d72e75672d94225580d7eecbcfcac2e9b3dbc0 From b5bc04f898e97b3569abe3fab2fa0a5da5aef4ce Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Mon, 24 Mar 2025 17:03:01 +0100 Subject: [PATCH 11/14] skip retry test on local testing --- tests/test_integration_crashpad.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_integration_crashpad.py b/tests/test_integration_crashpad.py index f835d6578..614cfd6ee 100644 --- a/tests/test_integration_crashpad.py +++ b/tests/test_integration_crashpad.py @@ -522,7 +522,8 @@ def test_disable_backend(cmake, httpserver): @pytest.mark.skipif( - sys.platform != "darwin", reason="retry mechanism test only runs on macOS" + sys.platform != "darwin" or not os.getenv("CI"), + reason="retry mechanism test only runs on macOS in CI", ) def test_crashpad_retry(cmake, httpserver): tmp_path = cmake(["sentry_example"], {"SENTRY_BACKEND": "crashpad"}) From 1c70c12410e0ee12f4bab2a43089c35c322bae58 Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Tue, 25 Mar 2025 11:47:38 +0100 Subject: [PATCH 12/14] Apply suggestions from code review Co-authored-by: Mischan Toosarani-Hausberger --- CHANGELOG.md | 2 +- CONTRIBUTING.md | 2 +- include/sentry.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c5b83d2b..e80b76a72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ **Features**: - Add option to attach screenshots on Windows to fatal error events. ([#1170](https://github.com/getsentry/sentry-native/pull/1170)) -- Add option for `Crashpad` to delay shutdown until upload thread completes ([#1153](https://github.com/getsentry/sentry-native/pull/1153)) +- Add an option for `Crashpad` on Linux to delay application shutdown until the upload of the crash report in the `crashpad_handler` is complete. This is useful for deployment in `Docker` or `systemd`, where the life cycle of additional processes is bound by the application life cycle. ([#1153](https://github.com/getsentry/sentry-native/pull/1153), [crashpad#121](https://github.com/getsentry/crashpad/pull/121)) ## 0.8.2 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b51c8fce6..1d576b78f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -159,7 +159,7 @@ The example currently supports the following commands: Only on Linux using crashpad: -- `crashpad-wait-for-upload`: Couples application shutdown to finishing the upload thread. +- `crashpad-wait-for-upload`: Couples application shutdown to complete the upload in the `crashpad_handler`. Only on Windows using crashpad with its WER handler module: diff --git a/include/sentry.h b/include/sentry.h index 03b4e8643..be4a886d5 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -1291,7 +1291,7 @@ SENTRY_API void sentry_options_set_system_crash_reporter_enabled( * Enables a wait for the crash report upload to be finished before shutting * down. This is disabled by default. * - * This setting only has an effect when using the crashpad backend. + * This setting only has an effect when using the `crashpad` backend on Linux. */ SENTRY_API void sentry_options_set_on_crash_wait_for_upload( sentry_options_t *opts, int wait_for_upload); From 4b09ff1590f9f1ff65a1b7102db86d0b91baba72 Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Tue, 25 Mar 2025 11:53:35 +0100 Subject: [PATCH 13/14] rename option to `crashpad_wait_for_upload` --- examples/example.c | 2 +- include/sentry.h | 2 +- src/backends/sentry_backend_crashpad.cpp | 2 +- src/sentry_options.c | 6 +++--- src/sentry_options.h | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/examples/example.c b/examples/example.c index 0078ba511..c2c953a94 100644 --- a/examples/example.c +++ b/examples/example.c @@ -287,7 +287,7 @@ main(int argc, char **argv) } if (has_arg(argc, argv, "crashpad-wait-for-upload")) { - sentry_options_set_on_crash_wait_for_upload(options, true); + sentry_options_set_crashpad_wait_for_upload(options, true); } sentry_init(options); diff --git a/include/sentry.h b/include/sentry.h index be4a886d5..c52d7336e 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -1293,7 +1293,7 @@ SENTRY_API void sentry_options_set_system_crash_reporter_enabled( * * This setting only has an effect when using the `crashpad` backend on Linux. */ -SENTRY_API void sentry_options_set_on_crash_wait_for_upload( +SENTRY_API void sentry_options_set_crashpad_wait_for_upload( sentry_options_t *opts, int wait_for_upload); /** diff --git a/src/backends/sentry_backend_crashpad.cpp b/src/backends/sentry_backend_crashpad.cpp index 7268db3e2..9ddca42fe 100644 --- a/src/backends/sentry_backend_crashpad.cpp +++ b/src/backends/sentry_backend_crashpad.cpp @@ -480,7 +480,7 @@ crashpad_backend_startup( minidump_url ? minidump_url : "", proxy_url, annotations, arguments, /* restartable */ true, /* asynchronous_start */ false, attachments, screenshot, - options->on_crash_wait_for_upload); + options->crashpad_wait_for_upload); sentry_free(minidump_url); #ifdef SENTRY_PLATFORM_WINDOWS diff --git a/src/sentry_options.c b/src/sentry_options.c index 3ee9b7b98..6a6b17429 100644 --- a/src/sentry_options.c +++ b/src/sentry_options.c @@ -48,7 +48,7 @@ sentry_options_new(void) opts->auto_session_tracking = true; opts->system_crash_reporter_enabled = false; opts->attach_screenshot = false; - opts->on_crash_wait_for_upload = false; + opts->crashpad_wait_for_upload = false; opts->symbolize_stacktraces = // AIX doesn't have reliable debug IDs for server-side symbolication, // and the diversity of Android makes it infeasible to have access to debug @@ -454,10 +454,10 @@ sentry_options_set_system_crash_reporter_enabled( } void -sentry_options_set_on_crash_wait_for_upload( +sentry_options_set_crashpad_wait_for_upload( sentry_options_t *opts, int wait_for_upload) { - opts->on_crash_wait_for_upload = !!wait_for_upload; + opts->crashpad_wait_for_upload = !!wait_for_upload; } void diff --git a/src/sentry_options.h b/src/sentry_options.h index dab2b7ee2..6ca477b43 100644 --- a/src/sentry_options.h +++ b/src/sentry_options.h @@ -49,7 +49,7 @@ struct sentry_options_s { bool symbolize_stacktraces; bool system_crash_reporter_enabled; bool attach_screenshot; - bool on_crash_wait_for_upload; + bool crashpad_wait_for_upload; sentry_attachment_t *attachments; sentry_run_t *run; From 3f932ff2bba439738a9f9a90b8a531748e9960d3 Mon Sep 17 00:00:00 2001 From: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com> Date: Tue, 25 Mar 2025 14:25:35 +0100 Subject: [PATCH 14/14] update crashpad --- external/crashpad | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/crashpad b/external/crashpad index 80d72e756..2d97a484b 160000 --- a/external/crashpad +++ b/external/crashpad @@ -1 +1 @@ -Subproject commit 80d72e75672d94225580d7eecbcfcac2e9b3dbc0 +Subproject commit 2d97a484bb628b05a46203e4a05c6c827796e5cd