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: Make SentryScope.useSpan non-blocking #3568

Merged
merged 1 commit into from Jan 24, 2024
Merged

fix: Make SentryScope.useSpan non-blocking #3568

merged 1 commit into from Jan 24, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jan 23, 2024

📜 Description

Instead of calling the callback of useSpan within the spanLock, get a reference to the span inside the critical sector and pass this to the callback after the critical sector closes. This way, when the callback is a blocking call, useSpan doesn't block other invocations of itself and prevents potential deadlocks.

💡 Motivation and Context

A customer reported a deadlock on macOS for which multiple threads pointed to acquiring locks to SentryScope useSpan.

💚 How did you test it?

Unit tests.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Instead of calling the callback of useSpan within the spanLock, get a
local copy of the span and pass this to the callback. This way, when the
callback is a blocking call, useSpan doesn't block other invocations of
itself and prevents potential deadlocks.
@philipphofmann philipphofmann marked this pull request as ready for review January 23, 2024 09:12
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (42ef6ba) 89.244% compared to head (70c017e) 89.223%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3568       +/-   ##
=============================================
- Coverage   89.244%   89.223%   -0.021%     
=============================================
  Files          529       529               
  Lines        57689     57692        +3     
  Branches     20641     20636        -5     
=============================================
- Hits         51484     51475        -9     
- Misses        5292      5300        +8     
- Partials       913       917        +4     
Files Coverage Δ
Sources/Sentry/SentryScope.m 96.791% <100.000%> (+0.017%) ⬆️
Tests/SentryTests/SentryScopeSwiftTests.swift 99.029% <100.000%> (+0.068%) ⬆️

... and 16 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42ef6ba...70c017e. Read the comment docs.

Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.92 ms 1242.69 ms 16.78 ms
Size 21.58 KiB 417.86 KiB 396.27 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1734d1b 1200.15 ms 1214.06 ms 13.92 ms
228a909 1248.92 ms 1267.42 ms 18.50 ms
d3edf46 1213.00 ms 1227.46 ms 14.46 ms
6e342ac 1220.24 ms 1240.14 ms 19.90 ms
be2977c 1202.51 ms 1221.32 ms 18.81 ms
e778bd2 1224.66 ms 1252.16 ms 27.50 ms
e998fd0 1241.49 ms 1262.63 ms 21.14 ms
72c8d84 1238.96 ms 1247.34 ms 8.38 ms
98a8c16 1234.69 ms 1265.02 ms 30.33 ms
b9d59f7 1250.71 ms 1257.78 ms 7.06 ms

App size

Revision Plain With Sentry Diff
1734d1b 21.58 KiB 418.82 KiB 397.23 KiB
228a909 20.76 KiB 425.71 KiB 404.95 KiB
d3edf46 20.76 KiB 436.65 KiB 415.89 KiB
6e342ac 20.76 KiB 436.66 KiB 415.90 KiB
be2977c 22.85 KiB 407.67 KiB 384.83 KiB
e778bd2 20.76 KiB 426.15 KiB 405.39 KiB
e998fd0 21.58 KiB 414.59 KiB 393.01 KiB
72c8d84 22.85 KiB 408.88 KiB 386.03 KiB
98a8c16 20.76 KiB 431.00 KiB 410.24 KiB
b9d59f7 22.85 KiB 405.77 KiB 382.93 KiB

@armcknight armcknight merged commit 28c80b5 into main Jan 24, 2024
71 of 73 checks passed
@armcknight armcknight deleted the fix/use-span branch January 24, 2024 19:28
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Make SentryScope.useSpan non-blocking ([#3568](https://github.com/getsentry/sentry-cocoa/pull/3568))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 70c017e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants