ci: enable tsan + fix: rework breadcrumb ringbuffer#1309
ci: enable tsan + fix: rework breadcrumb ringbuffer#1309JoshuaMoelans merged 32 commits intomasterfrom
Conversation
This will typically not hit because the inversion is relevant with the lock acquisition in `sentry_init()` and these two are typically not called from two different threads at the same time. However, consistent lock-order is such a basic sanity step and will guard us from future functions that also require scope- and options-locks.
…t unnecessary scope lock recursion
…global in the test-framework
this allows us to keep state close and remove the need to retrieve max_breadcrumbs from the options after sentry_init(). Which in turn removes the need for nested locks.
Nice! 🤩 |
Yeah, TSAN will prove very useful. This is particularly interesting because we knew that |
…hen forking the handler
I falsely assumed an issue in the signal handler itself because the internal check triggered by crashpad_handler forking coincided with DEADLYSIGNAL. Turns out DEADLYSIGNAL is handled in TSAN like in ASAN, i.e. non-fatal (puh). The culprit for crashpad is the fork during initialization, which breaks with TSAN invariants.
Also ensure that the cloned DSC is freed if we don't add it as a header.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1309 +/- ##
==========================================
+ Coverage 83.65% 83.84% +0.18%
==========================================
Files 54 55 +1
Lines 8286 8309 +23
Branches 1261 1264 +3
==========================================
+ Hits 6932 6967 +35
+ Misses 1235 1225 -10
+ Partials 119 117 -2 🚀 New features to boost your workflow:
|
|
This PR has broken compilation of unit tests on on switch and ps5. Do you know what needs to change out of the top of your head or do I need to investigate this properly? Probably related to SENTRY__MUTEX_INIT not being available on these platforms (they use _DYN). |
I am sorry, @vaind, I forgot to add the DYN case in the unit test. Essentially what needs to happen is to apply the same DYN init as in all other places we use |
|
Thanks for the info @supervacuus, I'll likely not get to do it today so feel free to make the changes and add me as a reviewer in the PR, I can verify downstream. |
* adapt python test runner * fix lock-order inversion in `sentry_start_session()` This will typically not hit because the inversion is relevant with the lock acquisition in `sentry_init()` and these two are typically not called from two different threads at the same time. However, consistent lock-order is such a basic sanity step and will guard us from future functions that also require scope- and options-locks. * properly synchronize `executed_after_shutdown` in `SENTRY_TEST(task_queue)` * move pre-init in sentry_init outside the options lock * move set_context outside the scope lock in set_trace so we can prevent unnecessary scope lock recursion * fix early exit in sentry__scope_get_span_or_transaction (only relevant for tests) * fix concurrent access to TEST_CHECK macro, which updates an unsynced global in the test-framework * minor format * extract the ringbuffer into a separate module this allows us to keep state close and remove the need to retrieve max_breadcrumbs from the options after sentry_init(). Which in turn removes the need for nested locks. * add "lock handling" to the contributor docs * synchronize access to acutest state in the concurrency unit tests * limit SENTRY_WITH_SCOPE in attachment tests to the lines that actually need the scope * introduce tsan configs into the matrix * remove ringbuffer get_len definition * add tsan.supp + support in test runner * disable tsan in integration tests that involve DEADLYSIGNALS * add GCC based TSAN tests as a toolchain cross-check * clean up * revert Werror for GCC due to crashpad (not solving this here) * increase TSAN verbosity for the internal check that fails * disable all crashpad runs for tsan since it fails an internal check when forking the handler * format * re-enable all crashing tests in the http/stdout integration tests I falsely assumed an issue in the signal handler itself because the internal check triggered by crashpad_handler forking coincided with DEADLYSIGNAL. Turns out DEADLYSIGNAL is handled in TSAN like in ASAN, i.e. non-fatal (puh). The culprit for crashpad is the fork during initialization, which breaks with TSAN invariants. * move ringbuffer tests in separate tu * missed cmake update * get rid of another unnecessary lock nesting * clone the DSC when prepping for the envelope header * clone the DSC when prepping for the envelope header (also in event path) * dramatically limit scope lock when prepping envelopes. Also ensure that the cloned DSC is freed if we don't add it as a header. * remove unused imports --------- Co-authored-by: JoshuaMoelans <60878493+JoshuaMoelans@users.noreply.github.com>
related to: #962 #974
Quick summary:
max_breadcrumbs(which is an init constant anyway)