Skip to content

Conversation

@philipphofmann
Copy link
Member

📜 Description

Replace the NSTimer with dispatch async, which we already use for the idle timeout and haven't had issues for a long time. Dispatch async also has the advantage that it can be called from any thread.

💡 Motivation and Context

Fixes GH-3320

💚 How did you test it?

Unit tests and simulator.l

📝 Checklist

You have to check all boxes before merging:

  • 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.
  • I updated the wizard 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

Replace the NSTimer with dispatch async, which we already use for the
idle timeout and haven't had issues for a long time. Dispatch async also
has the advantage that it can be called from any thread.

Fixes GH-3320
@codecov
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.278%. Comparing base (90afe02) to head (b558684).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4911       +/-   ##
=============================================
- Coverage   92.374%   92.278%   -0.097%     
=============================================
  Files          659       659               
  Lines        77547     77483       -64     
  Branches     28084     27267      -817     
=============================================
- Hits         71634     71500      -134     
- Misses        5817      5889       +72     
+ Partials        96        94        -2     
Files with missing lines Coverage Δ
Sources/Sentry/SentryTracer.m 97.520% <100.000%> (-0.509%) ⬇️
...urces/Sentry/SentryUIEventTrackerTransactionMode.m 100.000% <100.000%> (ø)
...SentryProfilerTests/SentryProfileTestFixture.swift 99.704% <ø> (-0.001%) ⬇️
...iewController/SentryTimeToDisplayTrackerTest.swift 100.000% <100.000%> (ø)
...egrations/UIEvents/SentryUIEventTrackerTests.swift 99.600% <100.000%> (ø)
...ts/SentryTests/Transaction/SentryTracerTests.swift 99.393% <100.000%> (+0.778%) ⬆️

... and 28 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 90afe02...b558684. Read the comment docs.

Copy link
Member

@philprime philprime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, while I do not fully understand the usage of the timer (yet), I did not spot any obvious issues with the changes.

Tests are still failing, so we should wait for CI to be green.

@github-actions
Copy link
Contributor

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1239.49 ms 1259.18 ms 19.69 ms
Size 22.31 KiB 821.12 KiB 798.82 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ea73af6 1241.69 ms 1252.96 ms 11.27 ms
9b9f2a0 1233.36 ms 1244.04 ms 10.68 ms
38b36f5 1218.48 ms 1246.61 ms 28.13 ms
443723a 1205.24 ms 1220.52 ms 15.28 ms
7cd187e 1239.02 ms 1261.42 ms 22.40 ms
ed64658 1225.06 ms 1247.73 ms 22.67 ms
0dfdaaa 1226.88 ms 1248.82 ms 21.94 ms
b35ccd0 1233.92 ms 1256.69 ms 22.78 ms
8cb9355 1225.23 ms 1231.22 ms 5.99 ms
4af7581 1214.55 ms 1237.10 ms 22.55 ms

App size

Revision Plain With Sentry Diff
ea73af6 20.76 KiB 425.76 KiB 404.99 KiB
9b9f2a0 21.58 KiB 707.42 KiB 685.84 KiB
38b36f5 21.58 KiB 616.35 KiB 594.77 KiB
443723a 20.76 KiB 414.44 KiB 393.68 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
ed64658 22.30 KiB 749.84 KiB 727.54 KiB
0dfdaaa 20.76 KiB 434.56 KiB 413.79 KiB
b35ccd0 21.58 KiB 573.13 KiB 551.55 KiB
8cb9355 20.76 KiB 419.70 KiB 398.95 KiB
4af7581 22.84 KiB 401.62 KiB 378.77 KiB

@philipphofmann philipphofmann merged commit 6d7f3a9 into main Feb 27, 2025
71 of 74 checks passed
@philipphofmann philipphofmann deleted the fix/deadline-timeout-crash branch February 27, 2025 13:32
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.

EXC_BAD_ACCESS in SentryTracer cancelDeadlineTimer

3 participants