Send envelopes that cannot be cached to disk (#4294)

Author: denrase

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### Features
 
 - Added `thermal_state` to device context (#4305)
+- Send envelopes that cannot be cached to disk (#4294)
 
 ### Refactoring
 

Sentry.xcodeproj/project.pbxproj

@@ -769,6 +769,7 @@
 		8EE017A126704CD500470616 /* SentryUIViewControllerPerformanceTrackerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8EA1ED0E2669152F00E62B98 /* SentryUIViewControllerPerformanceTrackerTests.swift */; };
 		8F0D6AA22B04115A00D048B1 /* SentryInstallation+Test.h in Sources */ = {isa = PBXBuildFile; fileRef = 8F0D6AA12B040A0100D048B1 /* SentryInstallation+Test.h */; };
 		8F73BC312B02B87E00C3CEF4 /* SentryInstallationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8F73BC302B02B87E00C3CEF4 /* SentryInstallationTests.swift */; };
+		92136D672C9D7660002A9FB8 /* SentryNSURLRequestBuilderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 92136D662C9D765D002A9FB8 /* SentryNSURLRequestBuilderTests.swift */; };
 		92672BB629C9A2A9006B021C /* SentryBreadcrumb+Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 92672BB529C9A2A9006B021C /* SentryBreadcrumb+Private.h */; };
 		9286059529A5096600F96038 /* SentryGeo.h in Headers */ = {isa = PBXBuildFile; fileRef = 9286059429A5096600F96038 /* SentryGeo.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		9286059729A5098900F96038 /* SentryGeo.m in Sources */ = {isa = PBXBuildFile; fileRef = 9286059629A5098900F96038 /* SentryGeo.m */; };
@@ -1836,6 +1837,7 @@
 		8F0D6AA12B040A0100D048B1 /* SentryInstallation+Test.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "SentryInstallation+Test.h"; sourceTree = "<group>"; };
 		8F73BC302B02B87E00C3CEF4 /* SentryInstallationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryInstallationTests.swift; sourceTree = "<group>"; };
 		8FF94DF22B06A24C00BCD650 /* SentryCrash+Test.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "SentryCrash+Test.h"; sourceTree = "<group>"; };
+		92136D662C9D765D002A9FB8 /* SentryNSURLRequestBuilderTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryNSURLRequestBuilderTests.swift; sourceTree = "<group>"; };
 		92672BB529C9A2A9006B021C /* SentryBreadcrumb+Private.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = "SentryBreadcrumb+Private.h"; path = "include/HybridPublic/SentryBreadcrumb+Private.h"; sourceTree = "<group>"; };
 		9286059429A5096600F96038 /* SentryGeo.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = SentryGeo.h; path = Public/SentryGeo.h; sourceTree = "<group>"; };
 		9286059629A5098900F96038 /* SentryGeo.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SentryGeo.m; sourceTree = "<group>"; };
@@ -3037,6 +3039,7 @@
 		7BBD18AF24517E5D00427C76 /* Networking */ = {
 			isa = PBXGroup;
 			children = (
+				92136D662C9D765D002A9FB8 /* SentryNSURLRequestBuilderTests.swift */,
 				7BBD18B124517FE800427C76 /* RateLimits */,
 				7BC8523A2458849E005A70F0 /* SentryDataCategoryMapperTests.swift */,
 				7B58816627FC5D790098B121 /* SentryDiscardReasonMapperTests.swift */,
@@ -4983,6 +4986,7 @@
 				D86130122BB563FD004C0F5E /* SentrySessionReplayIntegrationTests.swift in Sources */,
 				7BFC16BA2524D4AF00FF6266 /* SentryMessage+Equality.m in Sources */,
 				7B4260342630315C00B36EDD /* SampleError.swift in Sources */,
+				92136D672C9D7660002A9FB8 /* SentryNSURLRequestBuilderTests.swift in Sources */,
 				D855B3E827D652AF00BCED76 /* SentryCoreDataTrackingIntegrationTest.swift in Sources */,
 				D855AD62286ED6A4002573E1 /* SentryCrashTests.m in Sources */,
 				D8AFC0012BD252B900118BE1 /* SentryOnDemandReplayTests.swift in Sources */,

SentryTestUtils/TestClient.swift

@@ -144,10 +144,23 @@ public class TestFileManager: SentryFileManager {
     var storeTimestampLastInForegroundInvocations: Int = 0
     var deleteTimestampLastInForegroundInvocations: Int = 0
 
+    public var storeEnvelopeInvocations = Invocations<SentryEnvelope>()
+    public var storeEnvelopePath: String?
+    public var storeEnvelopePathNil: Bool = false
+    
     public init(options: Options) throws {
         try super.init(options: options, dispatchQueueWrapper: TestSentryDispatchQueueWrapper())
     }
 
+    public override func store(_ envelope: SentryEnvelope) -> String? {
+        storeEnvelopeInvocations.record(envelope)
+        if storeEnvelopePathNil {
+            return nil
+        } else {
+            return storeEnvelopePath ?? super.store(envelope)
+        }
+    }
+    
     public var deleteOldEnvelopeItemsInvocations = Invocations<Void>()
     public override func deleteOldEnvelopeItems() {
         deleteOldEnvelopeItemsInvocations.record(Void())

Sources/Sentry/SentryFileManager.m

@@ -319,6 +319,7 @@ - (nullable NSString *)storeEnvelope:(SentryEnvelope *)envelope
 
         if (![self writeData:envelopeData toPath:path]) {
             SENTRY_LOG_WARN(@"Failed to store envelope.");
+            return nil;
         }
 
         [self handleEnvelopesLimit];

Sources/Sentry/SentryHttpTransport.m

@@ -53,6 +53,8 @@ @interface SentryHttpTransport ()
 @property (nonatomic, strong)
     NSMutableDictionary<NSString *, SentryDiscardedEvent *> *discardedEvents;
 
+@property (nonatomic, strong) NSMutableArray<SentryEnvelope *> *notStoredEnvelopes;
+
 /**
  * Synching with a dispatch queue to have concurrent reads and writes as barrier blocks is roughly
  * 30% slower than using atomic here.
@@ -87,6 +89,7 @@ - (id)initWithOptions:(SentryOptions *)options
         _isSending = NO;
         _isFlushing = NO;
         self.discardedEvents = [NSMutableDictionary new];
+        self.notStoredEnvelopes = [NSMutableArray new];
         [self.envelopeRateLimit setDelegate:self];
         [self.fileManager setDelegate:self];
 
@@ -132,7 +135,13 @@ - (void)sendEnvelope:(SentryEnvelope *)envelope
     // thread, which could be the main thread.
     __weak SentryHttpTransport *weakSelf = self;
     [self.dispatchQueue dispatchAsyncWithBlock:^{
-        [weakSelf.fileManager storeEnvelope:envelopeToStore];
+        NSString *path = [weakSelf.fileManager storeEnvelope:envelopeToStore];
+        if (path == nil) {
+            SENTRY_LOG_DEBUG(@"Could not store envelope. Schedule for sending.");
+            @synchronized(weakSelf.notStoredEnvelopes) {
+                [weakSelf.notStoredEnvelopes addObject:envelopeToStore];
+            }
+        }
         [weakSelf sendAllCachedEnvelopes];
     }];
 }
@@ -280,24 +289,38 @@ - (void)sendAllCachedEnvelopes
         self.isSending = YES;
     }
 
-    SentryFileContents *envelopeFileContents = [self.fileManager getOldestEnvelope];
-    if (nil == envelopeFileContents) {
-        SENTRY_LOG_DEBUG(@"No envelopes left to send.");
-        [self finishedSending];
-        return;
+    SentryEnvelope *envelope;
+    NSString *envelopeFilePath;
+
+    @synchronized(self.notStoredEnvelopes) {
+        if (self.notStoredEnvelopes.count > 0) {
+            envelope = self.notStoredEnvelopes[0];
+            [self.notStoredEnvelopes removeObjectAtIndex:0];
+        }
     }
 
-    SentryEnvelope *envelope = [SentrySerialization envelopeWithData:envelopeFileContents.contents];
-    if (nil == envelope) {
-        SENTRY_LOG_DEBUG(@"Envelope contained no deserializable data.");
-        [self deleteEnvelopeAndSendNext:envelopeFileContents.path];
-        return;
+    if (envelope == nil) {
+        SentryFileContents *envelopeFileContents = [self.fileManager getOldestEnvelope];
+        if (nil == envelopeFileContents) {
+            SENTRY_LOG_DEBUG(@"No envelopes left to send.");
+            [self finishedSending];
+            return;
+        }
+
+        envelopeFilePath = envelopeFileContents.path;
+
+        envelope = [SentrySerialization envelopeWithData:envelopeFileContents.contents];
+        if (nil == envelope) {
+            SENTRY_LOG_DEBUG(@"Envelope contained no deserializable data.");
+            [self deleteEnvelopeAndSendNext:envelopeFilePath];
+            return;
+        }
     }
 
     SentryEnvelope *rateLimitedEnvelope = [self.envelopeRateLimit removeRateLimitedItems:envelope];
     if (rateLimitedEnvelope.items.count == 0) {
         SENTRY_LOG_DEBUG(@"Envelope had no rate-limited items, nothing to send.");
-        [self deleteEnvelopeAndSendNext:envelopeFileContents.path];
+        [self deleteEnvelopeAndSendNext:envelopeFilePath];
         return;
     }
 
@@ -309,22 +332,24 @@ - (void)sendAllCachedEnvelopes
                                                                    dsn:self.options.parsedDsn
                                                       didFailWithError:&requestError];
 
-    if (nil != requestError) {
-        SENTRY_LOG_DEBUG(@"Failed to build request: %@.", requestError);
+    if (nil == request || nil != requestError) {
+        if (nil != requestError) {
+            SENTRY_LOG_DEBUG(@"Failed to build request: %@.", requestError);
+        }
         [self recordLostEventFor:rateLimitedEnvelope.items];
-        [self deleteEnvelopeAndSendNext:envelopeFileContents.path];
+        [self deleteEnvelopeAndSendNext:envelopeFilePath];
         return;
     } else {
-        [self sendEnvelope:rateLimitedEnvelope
-              envelopePath:envelopeFileContents.path
-                   request:request];
+        [self sendEnvelope:rateLimitedEnvelope envelopePath:envelopeFilePath request:request];
     }
 }
 
-- (void)deleteEnvelopeAndSendNext:(NSString *)envelopePath
+- (void)deleteEnvelopeAndSendNext:(nullable NSString *)envelopePath
 {
     SENTRY_LOG_DEBUG(@"Deleting envelope and sending next.");
-    [self.fileManager removeFileAtPath:envelopePath];
+    if (envelopePath != nil) {
+        [self.fileManager removeFileAtPath:envelopePath];
+    }
     @synchronized(self) {
         self.isSending = NO;
     }
@@ -340,7 +365,7 @@ - (void)deleteEnvelopeAndSendNext:(NSString *)envelopePath
 }
 
 - (void)sendEnvelope:(SentryEnvelope *)envelope
-        envelopePath:(NSString *)envelopePath
+        envelopePath:(NSString *_Nullable)envelopePath
              request:(NSURLRequest *)request
 {
     __weak SentryHttpTransport *weakSelf = self;

Sources/Sentry/SentryNSURLRequestBuilder.m

@@ -1,5 +1,6 @@
 #import "SentryNSURLRequestBuilder.h"
 #import "SentryDsn.h"
+#import "SentryLog.h"
 #import "SentryNSURLRequest.h"
 #import "SentrySerialization.h"
 #import <Foundation/Foundation.h>
@@ -8,25 +9,33 @@
 
 @implementation SentryNSURLRequestBuilder
 
-- (NSURLRequest *)createEnvelopeRequest:(SentryEnvelope *)envelope
-                                    dsn:(SentryDsn *)dsn
-                       didFailWithError:(NSError *_Nullable *_Nullable)error
+- (nullable NSURLRequest *)createEnvelopeRequest:(SentryEnvelope *)envelope
+                                             dsn:(SentryDsn *)dsn
+                                didFailWithError:(NSError *_Nullable *_Nullable)error
 {
-    return [[SentryNSURLRequest alloc]
-        initEnvelopeRequestWithDsn:dsn
-                           andData:[SentrySerialization dataWithEnvelope:envelope]
-                  didFailWithError:error];
+    NSData *data = [SentrySerialization dataWithEnvelope:envelope];
+    if (nil == data) {
+        SENTRY_LOG_ERROR(@"Envelope cannot be converted to data");
+        return nil;
+    }
+    return [[SentryNSURLRequest alloc] initEnvelopeRequestWithDsn:dsn
+                                                          andData:data
+                                                 didFailWithError:error];
 }
 
-- (NSURLRequest *)createEnvelopeRequest:(SentryEnvelope *)envelope
-                                    url:(NSURL *)url
-                       didFailWithError:(NSError *_Nullable *_Nullable)error
+- (nullable NSURLRequest *)createEnvelopeRequest:(SentryEnvelope *)envelope
+                                             url:(NSURL *)url
+                                didFailWithError:(NSError *_Nullable *_Nullable)error
 {
-    return [[SentryNSURLRequest alloc]
-        initEnvelopeRequestWithURL:url
-                           andData:[SentrySerialization dataWithEnvelope:envelope]
-                        authHeader:nil
-                  didFailWithError:error];
+    NSData *data = [SentrySerialization dataWithEnvelope:envelope];
+    if (nil == data) {
+        SENTRY_LOG_ERROR(@"Envelope cannot be converted to data");
+        return nil;
+    }
+    return [[SentryNSURLRequest alloc] initEnvelopeRequestWithURL:url
+                                                          andData:data
+                                                       authHeader:nil
+                                                 didFailWithError:error];
 }
 
 @end

Sources/Sentry/SentrySpotlightTransport.m

@@ -70,8 +70,10 @@ - (void)sendEnvelope:(SentryEnvelope *)envelope
                                                                    url:self.apiURL
                                                       didFailWithError:&requestError];
 
-    if (requestError) {
-        SENTRY_LOG_ERROR(@"Unable to build envelope request with error %@", requestError);
+    if (nil == request || nil != requestError) {
+        if (nil != requestError) {
+            SENTRY_LOG_ERROR(@"Unable to build envelope request with error %@", requestError);
+        }
         return;
     }
 

Sources/Sentry/include/SentryNSURLRequestBuilder.h

@@ -9,13 +9,13 @@ NS_ASSUME_NONNULL_BEGIN
  */
 @interface SentryNSURLRequestBuilder : NSObject
 
-- (NSURLRequest *)createEnvelopeRequest:(SentryEnvelope *)envelope
-                                    dsn:(SentryDsn *)dsn
-                       didFailWithError:(NSError *_Nullable *_Nullable)error;
+- (nullable NSURLRequest *)createEnvelopeRequest:(SentryEnvelope *)envelope
+                                             dsn:(SentryDsn *)dsn
+                                didFailWithError:(NSError *_Nullable *_Nullable)error;
 
-- (NSURLRequest *)createEnvelopeRequest:(SentryEnvelope *)envelope
-                                    url:(NSURL *)url
-                       didFailWithError:(NSError *_Nullable *_Nullable)error;
+- (nullable NSURLRequest *)createEnvelopeRequest:(SentryEnvelope *)envelope
+                                             url:(NSURL *)url
+                                didFailWithError:(NSError *_Nullable *_Nullable)error;
 
 @end
 

Tests/SentryTests/Networking/SentryHttpTransportTests.swift

@@ -125,19 +125,19 @@ class SentryHttpTransportTests: XCTestCase {
             return transactionEnvelope
         }
 
-        func  getSut(dispatchQueueWrapper: SentryDispatchQueueWrapper? = nil) -> SentryHttpTransport {
-            
-            let dispatchQueue = dispatchQueueWrapper ?? self.dispatchQueueWrapper
-            
+        func getSut(
+            fileManager: SentryFileManager? = nil,
+            dispatchQueueWrapper: SentryDispatchQueueWrapper? = nil
+        ) -> SentryHttpTransport {
             return SentryHttpTransport(
                 options: options,
                 cachedEnvelopeSendDelay: 0.0,
-                fileManager: fileManager,
+                fileManager: fileManager ?? self.fileManager,
                 requestManager: requestManager,
                 requestBuilder: requestBuilder,
                 rateLimits: rateLimits,
                 envelopeRateLimit: EnvelopeRateLimit(rateLimits: rateLimits),
-                dispatchQueueWrapper: dispatchQueue
+                dispatchQueueWrapper: dispatchQueueWrapper ?? self.dispatchQueueWrapper
             )
         }
     }
@@ -470,7 +470,7 @@ class SentryHttpTransportTests: XCTestCase {
     }
 
     func testAllCachedEnvelopesCantDeserializeEnvelope() throws {
-        let path = try XCTUnwrap( fixture.fileManager.store(TestConstants.envelope))
+        let path = try XCTUnwrap(fixture.fileManager.store(TestConstants.envelope))
         let faultyEnvelope = Data([0x70, 0xa3, 0x10, 0x45])
         try faultyEnvelope.write(to: URL(fileURLWithPath: path))
 
@@ -479,6 +479,17 @@ class SentryHttpTransportTests: XCTestCase {
         assertRequestsSent(requestCount: 1)
         assertEnvelopesStored(envelopeCount: 0)
     }
+    
+    func testFailureToStoreEvenlopeEventStillSendsRequest() throws {
+        let fileManger = try TestFileManager(options: fixture.options)
+        fileManger.storeEnvelopePathNil = true // Failure to store envelope returns nil path
+        let sut = fixture.getSut(fileManager: fileManger)
+                                              
+        sut.send(envelope: fixture.eventEnvelope)
+        
+        XCTAssertEqual(fileManger.storeEnvelopeInvocations.count, 1)
+        assertRequestsSent(requestCount: 1)
+    }
 
     func testSendCachedEnvelopesFirst() throws {
         givenNoInternetConnection()
@@ -651,6 +662,16 @@ class SentryHttpTransportTests: XCTestCase {
         assertRequestsSent(requestCount: 1)
     }
 
+    func testBuildingRequestFailsReturningNil_DeletesEnvelopeAndSendsNext() {
+        givenNoInternetConnection()
+        sendEvent()
+        
+        fixture.requestBuilder.shouldFailReturningNil = true
+        sendEvent()
+        assertEnvelopesStored(envelopeCount: 0)
+        assertRequestsSent(requestCount: 1)
+    }
+    
     func testDeallocated_CachedEnvelopesNotAllSent() throws {
         givenNoInternetConnection()
         givenCachedEvents(amount: 10)