RUMM-346 Catch NSExceptions in FileWriter

DataDog · Apr 6, 2020 · b5b28f6 · b5b28f6
1 parent 9e4fbc3
commit b5b28f6
Show file tree

Hide file tree

Showing 13 changed files with 153 additions and 44 deletions.
diff --git a/Datadog/Datadog.xcodeproj/project.pbxproj b/Datadog/Datadog.xcodeproj/project.pbxproj
@@ -89,6 +89,9 @@
 		61133C712423993200786299 /* Datadog.framework in Embed Frameworks */ = {isa = PBXBuildFile; fileRef = 61133B82242393DE00786299 /* Datadog.framework */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; };
 		616A62D024349BA700D1BE12 /* ObjcExceptionHandler.m in Sources */ = {isa = PBXBuildFile; fileRef = 616A626924335D4900D1BE12 /* ObjcExceptionHandler.m */; };
 		616A62D62434A3C500D1BE12 /* ObjcExceptionHandler.h in Headers */ = {isa = PBXBuildFile; fileRef = 616A626A24335D4900D1BE12 /* ObjcExceptionHandler.h */; };
+		61C363802436164B00C4D4E6 /* ObjcExceptionHandlerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61C3637F2436164B00C4D4E6 /* ObjcExceptionHandlerTests.swift */; };
+		61C3638324361BE200C4D4E6 /* DatadogPrivateMocks.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61C3638224361BE200C4D4E6 /* DatadogPrivateMocks.swift */; };
+		61C3638524361E9200C4D4E6 /* Globals.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61C3638424361E9200C4D4E6 /* Globals.swift */; };
 		9E08587A242519FF001A3583 /* NetworkPathMonitor.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9E085879242519FF001A3583 /* NetworkPathMonitor.swift */; };
 /* End PBXBuildFile section */
 
@@ -217,6 +220,9 @@
 		616A626924335D4900D1BE12 /* ObjcExceptionHandler.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = ObjcExceptionHandler.m; sourceTree = "<group>"; };
 		616A626A24335D4900D1BE12 /* ObjcExceptionHandler.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = ObjcExceptionHandler.h; sourceTree = "<group>"; };
 		616A62DF2434BEF200D1BE12 /* module.modulemap */ = {isa = PBXFileReference; lastKnownFileType = "sourcecode.module-map"; path = module.modulemap; sourceTree = "<group>"; };
+		61C3637F2436164B00C4D4E6 /* ObjcExceptionHandlerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ObjcExceptionHandlerTests.swift; sourceTree = "<group>"; };
+		61C3638224361BE200C4D4E6 /* DatadogPrivateMocks.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DatadogPrivateMocks.swift; sourceTree = "<group>"; };
+		61C3638424361E9200C4D4E6 /* Globals.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Globals.swift; sourceTree = "<group>"; };
 		9E085879242519FF001A3583 /* NetworkPathMonitor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NetworkPathMonitor.swift; sourceTree = "<group>"; };
 /* End PBXFileReference section */
 
@@ -369,6 +375,7 @@
 		61133BB72423979B00786299 /* Utils */ = {
 			isa = PBXGroup;
 			children = (
+				61C3638424361E9200C4D4E6 /* Globals.swift */,
 				61133BB82423979B00786299 /* InternalLoggers.swift */,
 				61133BB92423979B00786299 /* CompilationConditions.swift */,
 				61133BBA2423979B00786299 /* SwiftExtensions.swift */,
@@ -461,6 +468,7 @@
 			children = (
 				61133C182423990D00786299 /* Datadog */,
 				61133C132423990D00786299 /* DatadogObjc */,
+				61C3637E2436163400C4D4E6 /* DatadogPrivate */,
 				61133C422423990D00786299 /* Matchers */,
 				61133C442423990D00786299 /* Helpers */,
 			);
@@ -501,6 +509,7 @@
 				61133C1C2423990D00786299 /* UIKitMocks.swift */,
 				61133C1D2423990D00786299 /* DatadogObjcMocks.swift */,
 				61133C1F2423990D00786299 /* DatadogMocks.swift */,
+				61C3638224361BE200C4D4E6 /* DatadogPrivateMocks.swift */,
 				61133C202423990D00786299 /* FoundationMocks.swift */,
 			);
 			path = Mocks;
@@ -631,6 +640,14 @@
 			path = DatadogPrivate;
 			sourceTree = "<group>";
 		};
+		61C3637E2436163400C4D4E6 /* DatadogPrivate */ = {
+			isa = PBXGroup;
+			children = (
+				61C3637F2436164B00C4D4E6 /* ObjcExceptionHandlerTests.swift */,
+			);
+			path = DatadogPrivate;
+			sourceTree = "<group>";
+		};
 /* End PBXGroup section */
 
 /* Begin PBXHeadersBuildPhase section */
@@ -812,6 +829,7 @@
 				61133BCC2423979B00786299 /* MobileDevice.swift in Sources */,
 				9E08587A242519FF001A3583 /* NetworkPathMonitor.swift in Sources */,
 				61133BCA2423979B00786299 /* EncodableValue.swift in Sources */,
+				61C3638524361E9200C4D4E6 /* Globals.swift in Sources */,
 				616A62D024349BA700D1BE12 /* ObjcExceptionHandler.m in Sources */,
 				61133BE62423979B00786299 /* LogSanitizer.swift in Sources */,
 				61133BDF2423979B00786299 /* SwiftExtensions.swift in Sources */,
@@ -854,9 +872,11 @@
 				61133C622423990D00786299 /* InternalLoggersTests.swift in Sources */,
 				61133C582423990D00786299 /* FileWriterTests.swift in Sources */,
 				61133C672423990D00786299 /* LogConsoleOutputTests.swift in Sources */,
+				61C3638324361BE200C4D4E6 /* DatadogPrivateMocks.swift in Sources */,
 				61133C4C2423990D00786299 /* LogsMocks.swift in Sources */,
 				61133C542423990D00786299 /* NetworkConnectionInfoProviderTests.swift in Sources */,
 				61133C4A2423990D00786299 /* DDConfigurationTests.swift in Sources */,
+				61C363802436164B00C4D4E6 /* ObjcExceptionHandlerTests.swift in Sources */,
 				61133C602423990D00786299 /* HTTPHeadersTests.swift in Sources */,
 				61133C632423990D00786299 /* DatadogConfigurationTests.swift in Sources */,
 				61133C572423990D00786299 /* FileReaderTests.swift in Sources */,

diff --git a/Datadog/DatadogPrivate/ObjcExceptionHandler.h b/Datadog/DatadogPrivate/ObjcExceptionHandler.h
@@ -10,7 +10,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 @interface ObjcExceptionHandler : NSObject
 
-+ (BOOL)catchException:(void(^)(void))tryBlock error:(__autoreleasing NSError **)error
+- (BOOL)catchException:(void(^)(void))tryBlock error:(__autoreleasing NSError **)error
     NS_SWIFT_NAME(rethrowToSwift(tryBlock:));
 
 @end

diff --git a/Datadog/DatadogPrivate/ObjcExceptionHandler.m b/Datadog/DatadogPrivate/ObjcExceptionHandler.m
@@ -9,7 +9,7 @@
 
 @implementation ObjcExceptionHandler
 
-+ (BOOL)catchException:(void(^)(void))tryBlock error:(__autoreleasing NSError **)error {
+- (BOOL)catchException:(void(^)(void))tryBlock error:(__autoreleasing NSError **)error {
     @try {
         tryBlock();
         return YES;

diff --git a/Sources/Datadog/Core/Persistence/FileWriter.swift b/Sources/Datadog/Core/Persistence/FileWriter.swift
@@ -51,13 +51,13 @@ internal final class FileWriter {
             let file = try orchestrator.getWritableFile(writeSize: UInt64(data.count))
 
             if try file.size() == 0 {
-                try file.append { write in
-                    write(data)
+                try file.append { (write: (Data) throws -> Void) in
+                    try write(data)
                 }
             } else {
                 try file.append { write in
-                    write(commaSeparatorData)
-                    write(data)
+                    try write(commaSeparatorData)
+                    try write(data)
                 }
             }
         } catch {

diff --git a/Sources/Datadog/Core/Persistence/Files/File.swift b/Sources/Datadog/Core/Persistence/Files/File.swift
@@ -5,6 +5,7 @@
  */
 
 import Foundation
+import _Datadog_Private
 
 /// Provides convenient interface for reading metadata and appending data to the file.
 internal protocol WritableFile {
@@ -15,7 +16,7 @@ internal protocol WritableFile {
     func size() throws -> UInt64
 
     /// Synchronously appends given data at the end of this file.
-    func append(transaction: ((Data) -> Void) -> Void) throws
+    func append(transaction: ((Data) throws -> Void) throws -> Void) throws
 }
 
 /// Provides convenient interface for reading contents and metadata of the file.
@@ -42,27 +43,24 @@ internal struct File: WritableFile, ReadableFile {
     }
 
     /// Appends given data at the end of this file.
-    func append(transaction: ((Data) -> Void) -> Void) throws {
+    func append(transaction: ((Data) throws -> Void) throws -> Void) throws {
         let fileHandle = try FileHandle(forWritingTo: url)
         defer { fileHandle.closeFile() }
-        fileHandle.seekToEndOfFile()
+
+        try objcExceptionHandler.rethrowToSwift {
+            fileHandle.seekToEndOfFile()
+        }
 
         // Writes given data at the end of the file.
-        func appendData(_ data: Data) {
-            /*
-             Apple documentation https://developer.apple.com/documentation/foundation/filehandle/1410936-write says
-             that `fileHandle.write()` raises an exception if no free space is left on the file system,
-             or if any other writing error occurs. Those are unchecked exceptions impossible to handle in Swift.
-
-             It was already escalated to Apple in Swift open source project discussion:
-             https://forums.swift.org/t/pitch-replacement-for-filehandle/5177
-
-             Until better replacement is provided by Apple, the SDK sticks to this API. To mitigate the risk of
-             crashing client applications, other precautions are implemented carefuly.
-             */
-            fileHandle.write(data)
+        func appendData(_ data: Data) throws {
+            try objcExceptionHandler.rethrowToSwift {
+                fileHandle.write(data)
+            }
+        }
+
+        try transaction { chunkOfData in
+            try appendData(chunkOfData)
         }
-        transaction(appendData)
     }
 
     func read() throws -> Data {

diff --git a/Sources/Datadog/Logs/LogOutputs/LogConsoleOutput.swift b/Sources/Datadog/Logs/LogOutputs/LogConsoleOutput.swift
@@ -11,11 +11,6 @@ internal protocol ConsoleLogFormatter {
     func format(log: Log) -> String
 }
 
-/// Function printing `String` content to console.
-internal var consolePrint: (String) -> Void = { content in
-    print(content)
-}
-
 /// `LogOutput` which prints logs to console.
 internal struct LogConsoleOutput: LogOutput {
     /// Time formatter used for `.short` output format.

diff --git a/Sources/Datadog/Utils/Globals.swift b/Sources/Datadog/Utils/Globals.swift
@@ -0,0 +1,15 @@
+/*
+ * Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
+ * This product includes software developed at Datadog (https://www.datadoghq.com/).
+ * Copyright 2019-2020 Datadog, Inc.
+ */
+
+import _Datadog_Private
+
+/// Function printing `String` content to console.
+internal var consolePrint: (String) -> Void = { content in
+    print(content)
+}
+
+/// Exception handler rethrowing `NSExceptions` to Swift `NSError`.
+internal var objcExceptionHandler = ObjcExceptionHandler()
diff --git a/Tests/DatadogTests/Datadog/Core/Persistence/FileReaderTests.swift b/Tests/DatadogTests/Datadog/Core/Persistence/FileReaderTests.swift
@@ -27,7 +27,7 @@ class FileReaderTests: XCTestCase {
         )
         _ = try temporaryDirectory
             .createFile(named: .mockAnyFileName())
-            .append { write in write("ABCD".utf8Data) }
+            .append { write in try write("ABCD".utf8Data) }
 
         let batch = reader.readNextBatch()
 
@@ -46,13 +46,13 @@ class FileReaderTests: XCTestCase {
             queue: queue
         )
         let file1 = try temporaryDirectory.createFile(named: dateProvider.currentDate().toFileName)
-        try file1.append { write in write("1".utf8Data) }
+        try file1.append { write in try write("1".utf8Data) }
 
         let file2 = try temporaryDirectory.createFile(named: dateProvider.currentDate().toFileName)
-        try file2.append { write in write("2".utf8Data) }
+        try file2.append { write in try write("2".utf8Data) }
 
         let file3 = try temporaryDirectory.createFile(named: dateProvider.currentDate().toFileName)
-        try file3.append { write in write("3".utf8Data) }
+        try file3.append { write in try write("3".utf8Data) }
 
         var batch: Batch
         batch = try reader.readNextBatch().unwrapOrThrow()

diff --git a/Tests/DatadogTests/Datadog/Core/Persistence/FileWriterTests.swift b/Tests/DatadogTests/Datadog/Core/Persistence/FileWriterTests.swift
@@ -63,7 +63,7 @@ class FileWriterTests: XCTestCase {
         XCTAssertEqual(try temporaryDirectory.files()[0].read(), #"{"key1":"value1"}"#.utf8Data) // same content as before
     }
 
-    func testGivenErrorVerbosity_whenLogCannotBeEncoded_itPrintsError() throws {
+    func testGivenErrorVerbosity_whenDataCannotBeEncoded_itPrintsError() throws {
         let expectation = self.expectation(description: "write completed")
         let previousUserLogger = userLogger
         defer { userLogger = previousUserLogger }
@@ -86,6 +86,32 @@ class FileWriterTests: XCTestCase {
         XCTAssertEqual(output.recordedLog?.message, "🔥 Failed to write log: failed to encode `FailingEncodable`.")
     }
 
+    func testGivenErrorVerbosity_whenIOExceptionIsThrown_itPrintsError() throws {
+        let expectation = self.expectation(description: "write completed")
+        let previousUserLogger = userLogger
+        defer { userLogger = previousUserLogger }
+        let previousObjcExceptionHandler = objcExceptionHandler
+        defer { objcExceptionHandler = previousObjcExceptionHandler }
+
+        let output = LogOutputMock()
+        userLogger = Logger(logOutput: output, identifier: "sdk-user")
+        objcExceptionHandler = ObjcExceptionHandlerMock(throwingError: ErrorMock("I/O exception"))
+
+        let writer = FileWriter(
+            orchestrator: .mockWriteToSingleFile(in: temporaryDirectory),
+            queue: queue,
+            maxWriteSize: .max
+        )
+
+        writer.write(value: ["whatever"])
+
+        waitForWritesCompletion(on: queue, thenFulfill: expectation)
+        waitForExpectations(timeout: 1, handler: nil)
+
+        XCTAssertEqual(output.recordedLog?.level, .error)
+        XCTAssertEqual(output.recordedLog?.message, "🔥 Failed to write log: I/O exception")
+    }
+
     private func waitForWritesCompletion(on queue: DispatchQueue, thenFulfill expectation: XCTestExpectation) {
         queue.async { expectation.fulfill() }
     }

diff --git a/Tests/DatadogTests/Datadog/Core/Persistence/Files/FileTests.swift b/Tests/DatadogTests/Datadog/Core/Persistence/Files/FileTests.swift
@@ -24,7 +24,7 @@ class FileTests: XCTestCase {
         let file = try temporaryDirectory.createFile(named: "file")
 
         try file.append { write in
-            write(Data([0x41, 0x41, 0x41, 0x41, 0x41])) // 5 bytes
+            try write(Data([0x41, 0x41, 0x41, 0x41, 0x41])) // 5 bytes
         }
 
         XCTAssertEqual(
@@ -33,8 +33,8 @@ class FileTests: XCTestCase {
         )
 
         try file.append { write in
-            write(Data([0x42, 0x42, 0x42, 0x42, 0x42])) // 5 bytes
-            write(Data([0x41, 0x41, 0x41, 0x41, 0x41])) // 5 bytes
+            try write(Data([0x42, 0x42, 0x42, 0x42, 0x42])) // 5 bytes
+            try write(Data([0x41, 0x41, 0x41, 0x41, 0x41])) // 5 bytes
         }
 
         XCTAssertEqual(
@@ -51,7 +51,7 @@ class FileTests: XCTestCase {
 
     func testItReadsDataFromFile() throws {
         let file = try temporaryDirectory.createFile(named: "file")
-        try file.append { write in write("Hello 👋".utf8Data) }
+        try file.append { write in try write("Hello 👋".utf8Data) }
 
         XCTAssertEqual(try file.read().utf8String, "Hello 👋")
     }
@@ -68,10 +68,10 @@ class FileTests: XCTestCase {
     func testItReturnsFileSize() throws {
         let file = try temporaryDirectory.createFile(named: "file")
 
-        try file.append { write in write(.mock(ofSize: 5)) }
+        try file.append { write in try write(.mock(ofSize: 5)) }
         XCTAssertEqual(try file.size(), 5)
 
-        try file.append { write in write(.mock(ofSize: 10)) }
+        try file.append { write in try write(.mock(ofSize: 10)) }
         XCTAssertEqual(try file.size(), 15)
     }
 }
diff --git a/Tests/DatadogTests/Datadog/Core/Persistence/FilesOrchestratorTests.swift b/Tests/DatadogTests/Datadog/Core/Persistence/FilesOrchestratorTests.swift
@@ -73,7 +73,7 @@ class FilesOrchestratorTests: XCTestCase {
         let chunkedData: [Data] = .mockChunksOf(totalSize: defaultWriteConditions.maxFileSize)
 
         let file1 = try orchestrator.getWritableFile(writeSize: defaultWriteConditions.maxFileSize)
-        try file1.append { write in chunkedData.forEach { chunk in write(chunk) } }
+        try file1.append { write in try chunkedData.forEach { chunk in try write(chunk) } }
         let file2 = try orchestrator.getWritableFile(writeSize: 1)
 
         XCTAssertNotEqual(file1.name, file2.name)
@@ -132,15 +132,15 @@ class FilesOrchestratorTests: XCTestCase {
 
         // write 1MB to first file (1MB of directory size in total)
         let file1 = try orchestrator.getWritableFile(writeSize: oneMB)
-        try file1.append { write in write(.mock(ofSize: oneMB)) }
+        try file1.append { write in try write(.mock(ofSize: oneMB)) }
 
         // write 1MB to second file (2MB of directory size in total)
         let file2 = try orchestrator.getWritableFile(writeSize: oneMB)
-        try file2.append { write in write(.mock(ofSize: oneMB)) }
+        try file2.append { write in try write(.mock(ofSize: oneMB)) }
 
         // write 1MB to third file (3MB of directory size in total)
         let file3 = try orchestrator.getWritableFile(writeSize: oneMB + 1) // +1 byte to exceed the limit
-        try file3.append { write in write(.mock(ofSize: oneMB + 1)) }
+        try file3.append { write in try write(.mock(ofSize: oneMB + 1)) }
 
         XCTAssertEqual(try temporaryDirectory.files().count, 3)
 
@@ -149,7 +149,7 @@ class FilesOrchestratorTests: XCTestCase {
         let file4 = try orchestrator.getWritableFile(writeSize: oneMB)
         XCTAssertEqual(try temporaryDirectory.files().count, 3)
         XCTAssertNil(try? temporaryDirectory.file(named: file1.name))
-        try file4.append { write in write(.mock(ofSize: oneMB + 1)) }
+        try file4.append { write in try write(.mock(ofSize: oneMB + 1)) }
 
         _ = try orchestrator.getWritableFile(writeSize: oneMB)
         XCTAssertEqual(try temporaryDirectory.files().count, 3)

diff --git a/Tests/DatadogTests/Datadog/Mocks/DatadogPrivateMocks.swift b/Tests/DatadogTests/Datadog/Mocks/DatadogPrivateMocks.swift
@@ -0,0 +1,23 @@
+/*
+ * Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
+ * This product includes software developed at Datadog (https://www.datadoghq.com/).
+ * Copyright 2019-2020 Datadog, Inc.
+ */
+
+import _Datadog_Private
+
+/*
+ A collection of mocks for `_Datadog_Private` module.
+ */
+
+class ObjcExceptionHandlerMock: ObjcExceptionHandler {
+    let error: Error
+
+    init(throwingError: Error) {
+        self.error = throwingError
+    }
+
+    override func rethrowToSwift(tryBlock: @escaping () -> Void) throws {
+        throw error
+    }
+}