Skip to content

refactor: Add factory for SentryClient to dependency container #5315

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

refactor: Add factory for SentryClient to dependency container #5315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion Sources/Sentry/SentryANRTrackingIntegration.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,14 @@
[SentryDependencyContainer.sharedInstance getANRTracker:options.appHangTimeoutInterval];

#endif // SENTRY_HAS_UIKIT
self.fileManager = SentryDependencyContainer.sharedInstance.fileManager;
SentryFileManager *_Nullable fileManager =
[SentryDependencyContainer.sharedInstance getFileManagerForOptions:options];
if (!fileManager) {
SENTRY_LOG_ERROR(
@"Failed to install ANR tracking integration, because file manager is not available.");
return NO;

Check warning on line 66 in Sources/Sentry/SentryANRTrackingIntegration.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryANRTrackingIntegration.m#L61-L66 

Added lines #L61 - L66 were not covered by tests
}
self.fileManager = fileManager;

Check warning on line 68 in Sources/Sentry/SentryANRTrackingIntegration.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryANRTrackingIntegration.m#L68 

Added line #L68 was not covered by tests
self.dispatchQueueWrapper = SentryDependencyContainer.sharedInstance.dispatchQueueWrapper;
self.crashWrapper = SentryDependencyContainer.sharedInstance.crashWrapper;
[self.tracker addListener:self];
Expand Down
25 changes: 6 additions & 19 deletions Sources/Sentry/SentryClient.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,27 +72,14 @@

- (_Nullable instancetype)initWithOptions:(SentryOptions *)options
{
return [self initWithOptions:options
dispatchQueue:[[SentryDispatchQueueWrapper alloc] init]
deleteOldEnvelopeItems:YES];
}

/** Internal constructor for testing purposes. */
- (nullable instancetype)initWithOptions:(SentryOptions *)options
dispatchQueue:(SentryDispatchQueueWrapper *)dispatchQueue
deleteOldEnvelopeItems:(BOOL)deleteOldEnvelopeItems
{
NSError *error;
SentryFileManager *fileManager = [[SentryFileManager alloc] initWithOptions:options
dispatchQueueWrapper:dispatchQueue
error:&error];
if (error != nil) {
SENTRY_LOG_FATAL(@"Failed to initialize file system: %@", error.localizedDescription);
SentryFileManager *fileManager =
[SentryDependencyContainer.sharedInstance getFileManagerForOptions:options];
if (fileManager == nil) {
SENTRY_LOG_FATAL(
@"Failed to initialize the client, because the file manager could not be created.");

Check warning on line 79 in Sources/Sentry/SentryClient.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryClient.m#L78-L79 

Added lines #L78 - L79 were not covered by tests
return nil;
}
return [self initWithOptions:options
fileManager:fileManager
deleteOldEnvelopeItems:deleteOldEnvelopeItems];
return [self initWithOptions:options fileManager:fileManager deleteOldEnvelopeItems:YES];
}

/** Internal constructor for testing purposes. */
Expand Down
32 changes: 26 additions & 6 deletions Sources/Sentry/SentryDependencyContainer.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#import "SentryANRTrackerV1.h"

#import "SentryBinaryImageCache.h"
#import "SentryClient.h"
#import "SentryDispatchFactory.h"
#import "SentryDispatchQueueWrapper.h"
#import "SentryDisplayLinkWrapper.h"
Expand All @@ -12,6 +13,7 @@
#import "SentryNSProcessInfoWrapper.h"
#import "SentryNSTimerFactory.h"
#import "SentryOptions+Private.h"
#import "SentryOptions.h"
#import "SentryRandom.h"
#import "SentrySDK+Private.h"
#import "SentrySwift.h"
Expand Down Expand Up @@ -172,16 +174,29 @@
- (SentryFileManager *)fileManager SENTRY_THREAD_SANITIZER_DOUBLE_CHECKED_LOCK
{
SENTRY_LAZY_INIT(_fileManager, ({
NSError *error;
SentryFileManager *manager = [[SentryFileManager alloc] initWithOptions:SentrySDK.options
error:&error];
if (manager == nil) {
SENTRY_LOG_DEBUG(@"Could not create file manager - %@", error);
SentryOptions *options = SentrySDK.options;
if (options == nil) {
SENTRY_LOG_FATAL(
@"SentryDependencyContainer.fileManager called before SentrySDK.options was set.");

Check warning on line 180 in Sources/Sentry/SentryDependencyContainer.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryDependencyContainer.m#L177-L180 

Added lines #L177 - L180 were not covered by tests
}
manager;
[self getFileManagerForOptions:options];

Check warning on line 182 in Sources/Sentry/SentryDependencyContainer.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryDependencyContainer.m#L182 

Added line #L182 was not covered by tests
}));
}

- (SentryFileManager *_Nullable)getFileManagerForOptions:(SentryOptions *)options
SENTRY_THREAD_SANITIZER_DOUBLE_CHECKED_LOCK
{
NSError *error;
SentryFileManager *manager =
[[SentryFileManager alloc] initWithOptions:options
dispatchQueueWrapper:self.dispatchQueueWrapper
error:&error];
if (manager == nil) {
SENTRY_LOG_FATAL(@"Could not create file manager - %@", error);

Check warning on line 195 in Sources/Sentry/SentryDependencyContainer.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryDependencyContainer.m#L195 

Added line #L195 was not covered by tests
}
return manager;
Comment on lines +189 to +197
Copy link
Member

Choose a reason for hiding this comment

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

h: The way the synchronization currently works in the SentryFileManager, we must ensure only having one SentryFileManager instance per DSN, as you already pointed out in the PR description. Otherwise, we end up with race conditions when accessing the files.
I think your idea of keeping weak references using the DSN with the key and the file manager as a value could work.

}

- (SentryAppStateManager *)appStateManager SENTRY_THREAD_SANITIZER_DOUBLE_CHECKED_LOCK
{
SENTRY_LAZY_INIT(_appStateManager,
Expand Down Expand Up @@ -347,4 +362,9 @@

#endif // SENTRY_HAS_METRIC_KIT

- (SentryClient *_Nullable)getClientWithOptions:(SentryOptions *)options
{
return [[SentryClient alloc] initWithOptions:options];

Check warning on line 367 in Sources/Sentry/SentryDependencyContainer.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryDependencyContainer.m#L366-L367 

Added lines #L366 - L367 were not covered by tests
}

@end
3 changes: 2 additions & 1 deletion Sources/Sentry/SentrySDK.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,8 @@
startInvocations++;
startTimestamp = [SentryDependencyContainer.sharedInstance.dateProvider date];

SentryClient *newClient = [[SentryClient alloc] initWithOptions:options];
SentryClient *newClient =
[SentryDependencyContainer.sharedInstance getClientWithOptions:options];

Check warning on line 241 in Sources/Sentry/SentrySDK.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentrySDK.m#L240-L241 

Added lines #L240 - L241 were not covered by tests
Comment on lines +240 to +241
Copy link
Member

Choose a reason for hiding this comment

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

m: I'm unsure if that solves anything, because any user can also manually init a client and we must support that use case. Using the SentryDependencyContainer internally increases complexity, and I don't see the benefit. What drove you adding this?

Copy link
Member

@armcknight armcknight May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

we must support that use case

is this an actual SDK requirement, or just due to the fact that we don't want to change the current public API in our implementation to avoid a major rev?

Copy link
Member

Choose a reason for hiding this comment

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

On the topic of what we're solving for here, I would be interested to know as well. AFAIUI this PR would help solve part of the file-backed data for watchdog events. Why do we need multiple instances of SentryFileManager for that?

Copy link
Member

Choose a reason for hiding this comment

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

is this an actual SDK requirement, or just due to the fact that we don't want to change the current public API in our implementation to avoid a major rev?

It's part of the unified API, which we deprecated some time ago, because we're merging the hub and the scope. Once we do the next major we can revisit if we want to keep the client public. There are some use cases for initializing your own client, which I can't recall from the top of my head. Most users on Cocoa interact with the static API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To summarize what we are trying to fix:

  • Our current Dependency Injection + Factory Pattern implementation is quite messy because of opportunistic refactoring.
  • Some instances are managed by integration instances
  • Some are part of the dependency injection container
  • Some are created using factories
  • Some instances in the static container are bounds to global options, but are used with local options (which can be different).
  • For local instances we fetch the dependencies from the container, then initialize it locally. This is duplicated in the code base, instead of delegating it to a factory with access to the dependency container (e.g. here and here.

For reference, if instances are properly managed in a dependency container the entire integration can be delegate to testable and mockable factories.

This technical documentation of Swinject might be helpful in seeing the advantages:

https://github.com/Swinject/Swinject/tree/master/Documentation

[newClient.fileManager moveAppStateToPreviousAppState];
[newClient.fileManager moveBreadcrumbsToPreviousBreadcrumbs];

Expand Down
20 changes: 14 additions & 6 deletions Sources/Sentry/include/HybridPublic/SentryDependencyContainer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
# import "SentryDefines.h"
#endif

@protocol SentryANRTracker;
@class SentryAppStateManager;
@class SentryBinaryImageCache;
@class SentryClient;
@class SentryCrash;
@class SentryCrashWrapper;
@class SentryDebugImageProvider;
Expand All @@ -17,12 +17,15 @@
@class SentryNSNotificationCenterWrapper;
@class SentryNSProcessInfoWrapper;
@class SentryNSTimerFactory;
@class SentryOptions;
@class SentrySwizzleWrapper;
@class SentrySysctl;
@class SentrySystemWrapper;
@class SentryThreadWrapper;
@class SentryThreadInspector;
@class SentryFileIOTracker;

@protocol SentryANRTracker;
@protocol SentryRandom;
@protocol SentryCurrentDateProvider;
@protocol SentryRateLimits;
Expand Down Expand Up @@ -102,11 +105,6 @@ SENTRY_NO_INIT
@property (nonatomic, strong) SentryFileIOTracker *fileIOTracker;
@property (nonatomic, strong) SentryCrash *crashReporter;

- (id<SentryANRTracker>)getANRTracker:(NSTimeInterval)timeout;
#if SENTRY_HAS_UIKIT
- (id<SentryANRTracker>)getANRTracker:(NSTimeInterval)timeout isV2Enabled:(BOOL)isV2Enabled;
#endif // SENTRY_HAS_UIKIT

@property (nonatomic, strong) SentrySystemWrapper *systemWrapper;
@property (nonatomic, strong) SentryDispatchFactory *dispatchFactory;
@property (nonatomic, strong) id<SentryDispatchQueueProviderProtocol> dispatchQueueProvider;
Expand All @@ -126,6 +124,16 @@ SENTRY_NO_INIT
ios(15.0), macos(12.0), macCatalyst(15.0)) API_UNAVAILABLE(tvos, watchos);
#endif // SENTRY_HAS_METRIC_KIT

#pragma mark - Factories

- (id<SentryANRTracker>)getANRTracker:(NSTimeInterval)timeout;
#if SENTRY_HAS_UIKIT
- (id<SentryANRTracker>)getANRTracker:(NSTimeInterval)timeout isV2Enabled:(BOOL)isV2Enabled;
#endif // SENTRY_HAS_UIKIT

- (SentryClient *_Nullable)getClientWithOptions:(SentryOptions *)options;
- (SentryFileManager *_Nullable)getFileManagerForOptions:(SentryOptions *)options;

@end

NS_ASSUME_NONNULL_END
Loading