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: SentryAppStateManager should always stop when closing the SDK #2460

Merged
merged 7 commits into from
Nov 29, 2022

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Nov 29, 2022

📜 Description

Force stop the SentryAppStateManager when calling SentrySDK.close(), even if you started the SDK multiple times.

💡 Motivation and Context

Closes #2455

💚 How did you test it?

Multiple unit tests. We have a test for SentryAppStateManager that tests the actual unsubscribing of the notification center, and an integration test that starts the SDK twice, stops it, and checks the start count of the SentryAppStateManager, which should now be zero. This test doesn't explicitly test the notification center unsubscribing as I can't really pass the mock center here. But we know that if the start count hits zero, that we do unsubscribe, that's what the first test checks.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link

github-actions bot commented Nov 29, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1226.80 ms 1253.16 ms 26.36 ms
Size 20.75 KiB 383.37 KiB 362.62 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
d10145a 1232.65 ms 1257.55 ms 24.90 ms
909e73a 1217.78 ms 1229.70 ms 11.92 ms
1ce879f 1258.12 ms 1260.90 ms 2.78 ms
3fdb749 1227.42 ms 1248.48 ms 21.06 ms
9f8d429 1239.57 ms 1255.12 ms 15.55 ms
9cb6c52 1201.08 ms 1211.37 ms 10.29 ms
e2cec76 1189.48 ms 1229.84 ms 40.36 ms
fb9f27a 1218.55 ms 1249.17 ms 30.62 ms
c9129b6 1231.86 ms 1270.11 ms 38.25 ms
dcac8ad 1238.82 ms 1247.80 ms 8.98 ms

App size

Revision Plain With Sentry Diff
d10145a 20.75 KiB 379.12 KiB 358.36 KiB
909e73a 20.75 KiB 383.40 KiB 362.65 KiB
1ce879f 20.75 KiB 381.81 KiB 361.06 KiB
3fdb749 20.75 KiB 383.81 KiB 363.06 KiB
9f8d429 20.75 KiB 383.40 KiB 362.65 KiB
9cb6c52 20.75 KiB 382.12 KiB 361.36 KiB
e2cec76 20.75 KiB 381.81 KiB 361.06 KiB
fb9f27a 20.75 KiB 382.11 KiB 361.36 KiB
c9129b6 20.75 KiB 381.81 KiB 361.06 KiB
dcac8ad 20.75 KiB 379.11 KiB 358.36 KiB

Previous results on branch: fix/2455-appstatemanager-stop

Startup times

Revision Plain With Sentry Diff
49fd801 1245.80 ms 1278.44 ms 32.64 ms
d253b96 1235.76 ms 1260.71 ms 24.95 ms
b4b7e62 1217.86 ms 1236.50 ms 18.64 ms
677447a 1226.74 ms 1241.28 ms 14.54 ms

App size

Revision Plain With Sentry Diff
49fd801 20.75 KiB 383.54 KiB 362.78 KiB
d253b96 20.75 KiB 383.43 KiB 362.67 KiB
b4b7e62 20.75 KiB 383.54 KiB 362.79 KiB
677447a 20.75 KiB 383.54 KiB 362.79 KiB

* 8.0.0:
  Merge branch 'master' into 8.0.0
  release: 7.31.3
  revert rename of sentryuser (#2462)
  fix: Reporting crashes when restarting the SDK (#2440)
  Update integration-tests.yml (#2461)
  fix: Core data span status with error (#2439)
  meta: disable swiftlint file length check in TBDBClient.swift (#2435)
  Revert "test: shorten some tests (#2422)" (#2427)
  test: shorten some tests (#2422)

# Conflicts:
#	Tests/SentryTests/Helper/TestNSNotificationCenterWrapper.swift
Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Just a question and suggestion but overall LGTM 👍🏻


#if SENTRY_HAS_UIKIT

- (void)start;
- (void)stop;
- (void)stop:(BOOL)forceStop;
Copy link
Member

Choose a reason for hiding this comment

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

Could you include a doc comment or rename this so that stop:NO/stop:YES is more obvious what it's doing at callsites?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (forceStop) {
self.startCount = 0;
} else {
self.startCount -= 1;
Copy link
Member

Choose a reason for hiding this comment

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

So if forceStop is not YES, then we require developers to correctly balance calls to start/stop? Why can't we just always reset startCount to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 2 integrations that start this manager. And only if both are stopped, should this manager be stopped as well. The only exception is if the entire SDK is closed, (which doesn't call the stop method on every integration). So then we force this to be stopped.

But yeah normally they have to be balanced.

@kevinrenskers kevinrenskers merged commit 65910c9 into 8.0.0 Nov 29, 2022
@kevinrenskers kevinrenskers deleted the fix/2455-appstatemanager-stop branch November 29, 2022 21:50
kevinrenskers added a commit that referenced this pull request Nov 29, 2022
…ERSIZE

* 8.0.0:
  fix: SentryAppStateManager should always stop when closing the SDK (#2460)
  Merge branch 'master' into 8.0.0
  release: 7.31.3
  fix: Reporting crashes when restarting the SDK (#2440)
  Update integration-tests.yml (#2461)
  fix: Core data span status with error (#2439)
  meta: disable swiftlint file length check in TBDBClient.swift (#2435)
  Revert "test: shorten some tests (#2422)" (#2427)
  test: shorten some tests (#2422)
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.

2 participants