Skip to content

FileLogger: use throwing FileHandle API so a failed write cannot cras… #31

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

Open
MartinPrusa wants to merge 1 commit into
base: master
Choose a base branch
from
Open

FileLogger: use throwing FileHandle API so a failed write cannot cras… #31

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
8 changes: 4 additions & 4 deletions Sources/Logger/Loggers/FileLogger/FileLogger.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@ public class FileLogger: Logging {
}
}

private var currentWritableFileHandle: FileHandle? {
var currentWritableFileHandle: FileHandle? {
willSet {
if currentWritableFileHandle != newValue {
currentWritableFileHandle?.closeFile()
try? currentWritableFileHandle?.close()
}
}
}
Expand Down Expand Up @@ -264,8 +264,8 @@ public class FileLogger: Logging {
throw FileLoggerError.stringToDataConversionFailure
}

fileHandle.seekToEndOfFile()
fileHandle.write(data)
try fileHandle.seekToEnd()
try fileHandle.write(contentsOf: data)
} catch let error {
self.externalLogger("Failed to write to a log file with error: \(error)!")
}
Expand Down
35 changes: 35 additions & 0 deletions Tests/LoggerTests/Loggers/FileLoggerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,41 @@ class FileLoggerTests: XCTestCase {
}
}

func test_failed_write_is_reported_instead_of_crashing() throws {

@David2XN David2XN Sep 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This does not test actual fix at all. I think verify this fix with a unit would be to difficult, so I propose simply removing this and merge it without test.

var internalErrors: [String] = []

let fileLogger = try FileLogger(
appName: nil,
fileManager: fileManager,
userDefaults: userDefaults,
logDirURL: logDirURL,
namespace: nil,
numberOfLogFiles: 3,
dateFormatter: DateFormatter.dateFormatter,
fileHeaderContent: "",
lineSeparator: "\n",
logEntryEncoder: LogEntryEncoder(),
logEntryDecoder: LogEntryDecoder(),
externalLogger: { internalErrors.append($0) },
fileAccessQueue: .syncMock
)

fileLogger.log(.mock("First message"))

// Simulate an I/O failure (e.g. "No space left on device") with a handle that cannot be written to.
// The legacy `seekToEndOfFile()` / `write(_:)` API raised an uncatchable NSException here and crashed the app.
fileLogger.currentWritableFileHandle = try FileHandle(forReadingFrom: fileLogger.currentLogFileUrl)

fileLogger.log(.mock("Second message"))

XCTAssertEqual(internalErrors.count, 1)
XCTAssertTrue(internalErrors.first?.hasPrefix("Failed to write to a log file") == true, internalErrors.description)

let content = try String(contentsOf: fileLogger.currentLogFileUrl, encoding: .utf8)
XCTAssertTrue(content.contains("First message"), content)
XCTAssertFalse(content.contains("Second message"), content)
}

func test_file_rotation() throws {
let fileLogger = try FileLogger(
appName: nil,
Expand Down