-
-
Notifications
You must be signed in to change notification settings - Fork 385
chore: Remove SentryDependencyContainer uses in SentryCrash
#7514
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
base: main
Are you sure you want to change the base?
Changes from 38 commits
811ed51
10c5625
19fc5ab
2065270
2bf41d4
ad95b86
8356b89
a0ba83e
02debdd
22d7fd2
f06b523
708b69a
55434e2
5d6fb07
48f185b
7bee0d1
d74a3f2
d658ec1
4e63349
51aa2e7
11bea7e
bb18a7f
eef3e2b
5c86043
96df891
cf45971
9cd8332
d59ab2a
599ee7b
4090604
b3301dc
648f2a2
5455e0f
039641b
4abc6c5
81c5db6
16cc298
806596e
a7bbc9c
f97754b
cbb6d35
04d2f72
df19413
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,9 @@ | |
| /** The exception handler that was in place before we installed ours. */ | ||
| static NSUncaughtExceptionHandler *g_previousUncaughtExceptionHandler; | ||
|
|
||
| /** Bridge for accessing SDK services. */ | ||
| static SentryCrashBridge *g_bridge = nil; | ||
|
|
||
| // ============================================================================ | ||
| #pragma mark - Callbacks - | ||
| // ============================================================================ | ||
|
|
@@ -113,6 +116,12 @@ | |
| #pragma mark - API - | ||
| // ============================================================================ | ||
|
|
||
| void | ||
| sentrycrashcm_nsexception_setBridge(SentryCrashBridge *bridge) | ||
| { | ||
| g_bridge = bridge; | ||
| } | ||
|
|
||
| static void | ||
| setEnabled(bool isEnabled) | ||
| { | ||
|
|
@@ -124,8 +133,7 @@ | |
|
|
||
| SENTRY_LOG_DEBUG(@"Setting new handler."); | ||
| NSSetUncaughtExceptionHandler(&handleUncaughtException); | ||
| SentryDependencyContainer.sharedInstance.crashReporter.uncaughtExceptionHandler | ||
| = &handleUncaughtException; | ||
| g_bridge.crashReporter.uncaughtExceptionHandler = &handleUncaughtException; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nil bridge silently skips uncaught exception handler registrationHigh Severity If |
||
| } else { | ||
| SENTRY_LOG_DEBUG(@"Restoring original handler."); | ||
| NSSetUncaughtExceptionHandler(g_previousUncaughtExceptionHandler); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| // swiftlint:disable missing_docs | ||
| @_implementationOnly import _SentryPrivate | ||
| import Foundation | ||
|
|
||
| #if (os(iOS) || os(tvOS)) && !SENTRY_NO_UI_FRAMEWORK | ||
| import UIKit | ||
| #endif | ||
|
|
||
| /** | ||
| * Concrete facade bridging SDK services to SentryCrash without requiring direct | ||
| * SentryDependencyContainer access. | ||
| * | ||
| * This class provides a single point of access for SentryCrash to SDK services, | ||
| * decoupling the crash reporting subsystem from the SDK's dependency container. | ||
| * It exposes only the five services SentryCrash needs: notification center wrapper, | ||
| * date provider, crash reporter, uncaught exception handler, and active screen size. | ||
| * | ||
| * The bridge follows the facade pattern established in the codebase, similar to | ||
| * SentryDependencyContainerSwiftHelper, providing a clean architectural boundary | ||
| * between layers. | ||
| */ | ||
| @objc @_spi(Private) public final class SentryCrashBridge: NSObject { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I would approach this by defining Swift protocols that describe the interaction between the Cocoa SDK and the crash reporting component / SentryCrash. These protocols should provide a generic crash-reporting interface without exposing SentryCrash-specific concepts. The Cocoa SDK would depend only on these protocols. We then introduce an adapter that implements the protocol and translates the generic crash reporting interface to SentryCrash. Ideally, this keeps changes inside SentryCrash minimal and avoids it needing to be aware that it runs inside Sentry Cocoa. Another significant reason for not making too many changes in SentryCrash is that we can port changes and fixes from KSCrash back to SentryCrash. The more we modify SentryCrash, the more it deviates from KSCrash, and the harder it will be to migrate changes back or to do a potential switch. Which then also applies to any other crash reporter that we might use in the future. Conceptually: Cocoa SDK → Crash Reporting Protocol → SentryCrashReportAdapter → SentryCrash Ideally, when you then swap out SentryCrash, for example, with KSCrash, you would only need to write a new adapter, but you don't need to change the crash reporting protocol. But I think it's okay if we're not fully there yet, because making that actually work is quite some effort. I think the main goal should be to have clear boundaries and make an optional switch in the future easier. If we, at some point, actually switch out the crash report with another one, for example, also on Swift Server only for Linux, it's acceptable to maybe have to adapt the protocol slightly, but it should not be too much effort. And coming back to what Phil said about SentryCrash being an external package, I think we should work towards that goal. When you could theoretically move SentryCrash into its own target or external package, we know we achieved this goal because the dependencies would be only from the Cocoa SDK to SentryCrash, not the other way around.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I definitely agree with that, I wanted to make a slow approach towards the protocol approach.
The PR title may have been misleading, this doesn't reach full isolation yet |
||
|
|
||
| // MARK: - Service Properties | ||
|
|
||
| /// Notification center wrapper for app lifecycle events | ||
| @objc public let notificationCenterWrapper: SentryNSNotificationCenterWrapper | ||
|
|
||
| /// Date provider for timestamps and system time | ||
| @objc public let dateProvider: SentryCurrentDateProvider | ||
|
|
||
| /// Crash reporter for system info and crash state | ||
| @objc public let crashReporter: SentryCrashSwift | ||
|
|
||
| /// Uncaught exception handler (bridges to crashReporter's property) | ||
| @objc public var uncaughtExceptionHandler: (@convention(c) (NSException) -> Void)? { | ||
| get { crashReporter.uncaughtExceptionHandler } | ||
| set { crashReporter.uncaughtExceptionHandler = newValue } | ||
| } | ||
|
|
||
| // MARK: - Platform-Specific Services | ||
|
|
||
| #if (os(iOS) || os(tvOS)) && !SENTRY_NO_UI_FRAMEWORK | ||
| /// Returns the active screen dimensions (iOS/tvOS only) | ||
| @objc public func activeScreenSize() -> CGSize { | ||
| return SentryDependencyContainerSwiftHelper.activeScreenSize() | ||
| } | ||
| #endif | ||
|
|
||
| // MARK: - Initialization | ||
|
|
||
| /// Initializes the bridge with required SDK services | ||
| /// - Parameters: | ||
| /// - notificationCenterWrapper: Wrapper for notification center operations | ||
| /// - dateProvider: Provider for current date and time operations | ||
| /// - crashReporter: Crash reporting service | ||
| @objc public init( | ||
| notificationCenterWrapper: SentryNSNotificationCenterWrapper, | ||
| dateProvider: SentryCurrentDateProvider, | ||
| crashReporter: SentryCrashSwift | ||
| ) { | ||
| self.notificationCenterWrapper = notificationCenterWrapper | ||
| self.dateProvider = dateProvider | ||
| self.crashReporter = crashReporter | ||
| super.init() | ||
| } | ||
| } | ||
| // swiftlint:enable missing_docs | ||


Uh oh!
There was an error while loading. Please reload this page.