diff --git a/OutputStreamError+PosixTests.swift b/OutputStreamError+PosixTests.swift new file mode 100644 index 00000000..e69de29b diff --git a/Sources/TraceLog/Utilities/Streams/FileOutputStream.swift b/Sources/TraceLog/Utilities/Streams/FileOutputStream.swift index dfec203d..868fbe15 100644 --- a/Sources/TraceLog/Utilities/Streams/FileOutputStream.swift +++ b/Sources/TraceLog/Utilities/Streams/FileOutputStream.swift @@ -113,11 +113,7 @@ internal class FileOutputStream { /// func close() { if self.fd >= 0 && closeFd { - #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS) - Darwin.close(self.fd) - #elseif os(Linux) || CYGWIN - Glibc.close(self.fd) - #endif + self.close(self.fd) self.fd = -1 } } @@ -129,7 +125,6 @@ internal class FileOutputStream { /// Should we close this file descriptor on deinit and on request or not. /// private let closeFd: Bool - } /// OutputStream conformance for FileOutputStream. @@ -158,18 +153,70 @@ extension FileOutputStream: OutputStream { /// Writes the byte block to the File. /// + /// - Note: We are forgoing locking of the file here since we write the data + /// nearly 100% of the time in a single write() call. POSIX guarantees that + /// if the file is opened in append mode, individual write() operations will + /// be atomic. Partial writes are the only time that we can't write in a + /// single call but with the minimal write sizes that are written, these + /// will be almost non-existent. + /// \ + /// Per the [POSIX standard](http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html): + /// \ + /// "If the O_APPEND flag of the file status flags is set, the file offset + /// shall be set to the end of the file prior to each write and no intervening + /// file modification operation shall occur between changing the file offset + /// and the write operation." + /// func write(_ bytes: [UInt8]) -> Result { + var buffer = UnsafePointer(bytes) + var length = bytes.count + + var written: Int = 0 + + /// Handle partial writes. + /// + repeat { + written = self.write(self.fd, buffer, length) + + if written == -1 { + if errno == EINTR { /// Always retry if interrupted. + continue + } + return .failure(OutputStreamError.error(for: errno)) + } + length -= written + buffer += written + + /// Exit if there are no more bytes (length != 0) or + /// we wrote zero bytes (written != 0) + /// + } while (length != 0 && written != 0) + + return .success(written) + } +} + +// Private extension to work around Swifts confusion around similar function names. +/// +private extension FileOutputStream { + + @inline(__always) + func write(_ fd: Int32, _ buffer: UnsafePointer, _ nbytes: Int) -> Int { #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS) - let written = Darwin.write(self.fd, UnsafePointer(bytes), bytes.count) + return Darwin.write(fd, buffer, nbytes) #elseif os(Linux) || CYGWIN - let written = Glibc.write(self.fd, UnsafePointer(bytes), bytes.count) + return Glibc.write(fd, buffer, nbytes) #endif + } - guard written != -1 - else { return .failure(OutputStreamError.error(for: errno)) } - - return .success(written) + @inline(__always) + func close(_ fd: Int32) { + #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS) + Darwin.close(fd) + #elseif os(Linux) || CYGWIN + Glibc.close(fd) + #endif } } diff --git a/Sources/TraceLog/Utilities/Streams/OutputStream.swift b/Sources/TraceLog/Utilities/Streams/OutputStream.swift index 8280dfc8..40f1d54e 100644 --- a/Sources/TraceLog/Utilities/Streams/OutputStream.swift +++ b/Sources/TraceLog/Utilities/Streams/OutputStream.swift @@ -50,12 +50,6 @@ internal enum OutputStreamError: Error { /// case networkDown(String) - /// The write operation was interrupted before it could be completed. - /// - /// - Note: this is a re-triable error case. - /// - case interrupted(String) - /// The stream was disconnected from its endpoint. /// case disconnected(String) diff --git a/Sources/TraceLog/Utilities/Streams/OutputStreamError+Posix.swift b/Sources/TraceLog/Utilities/Streams/OutputStreamError+Posix.swift index 7d2eed1f..32174d05 100644 --- a/Sources/TraceLog/Utilities/Streams/OutputStreamError+Posix.swift +++ b/Sources/TraceLog/Utilities/Streams/OutputStreamError+Posix.swift @@ -31,10 +31,6 @@ internal extension OutputStreamError { ENETUNREACH: /// A write was attempted on a socket and no route to the network is present. return .networkDown(message) - case EAGAIN, EWOULDBLOCK, /// The file descriptor is for a socket, is marked O_NONBLOCK, and write would block. - EINTR: /// The write operation was terminated due to the receipt of a signal, and no data was transferred. - return .interrupted(message) - case EBADF, /// The fd argument is not a valid file descriptor open for writing. EPIPE, /// A write was attempted on a socket that is shut down for writing, or is no longer connected. In the latter case, if the socket is of type SOCK_STREAM, a SIGPIPE signal shall also be sent to the thread. ECONNRESET, /// A write was attempted on a socket that is not connected. @@ -55,6 +51,9 @@ internal extension OutputStreamError { case EINVAL: /// The STREAM or multiplexer referenced by fd is linked (directly or indirectly) downstream from a multiplexer. return .invalidArgument(message) + case EAGAIN, EWOULDBLOCK, /// The file descriptor is for a socket, is marked O_NONBLOCK, and write would block. + EINTR: /// The write operation was terminated due to the receipt of a signal, and no data was transferred. + fallthrough default: return .unknownError(errorNumber, message) } diff --git a/Tests/TraceLogTests/Utilities/Streams/FileOutputStreamTests.swift b/Tests/TraceLogTests/Utilities/Streams/FileOutputStreamTests.swift index 1ad0fbf2..f1db9ef5 100644 --- a/Tests/TraceLogTests/Utilities/Streams/FileOutputStreamTests.swift +++ b/Tests/TraceLogTests/Utilities/Streams/FileOutputStreamTests.swift @@ -91,26 +91,83 @@ class FileOutputStreamTests: XCTestCase { // MARK: write tests - /// Test that a FileOutputStream.position returns the correct value after writing to a file. - /// func testWriteToFile() throws { let inputBytes = Array(repeating: 128, count: 10) + try self._testWrite(with: inputBytes) + } + + func testWriteWithSystemPageSizes() throws { + + let pageSizes = [1024, 4 * 1024, Int(PIPE_BUF)] + + for size in pageSizes { + try self._testWrite(with: Array(repeating: 128, count: size)) + } + } + + func testWriteWithJustOverSystemPageSizes() throws { + + let pageSizes = [1024, 4 * 1024, Int(PIPE_BUF)] + + for size in pageSizes { + try self._testWrite(with: Array(repeating: 128, count: size + 1)) + } + } + + func testWriteWithLargeWrites() throws { + + try self._testWrite(with: Array(repeating: 128, count: 1024 * 1024 + 1)) + } + + private func _testWrite(with bytes: [UInt8], file: StaticString = #file, line: UInt = #line) throws { + let inputURL = self.temporaryFileURL() defer { self.removeFileIfExists(url: inputURL) } let stream = try FileOutputStream(url: inputURL, options: [.create]) - switch stream.write(inputBytes) { + switch stream.write(bytes) { case .success(let written): - XCTAssertEqual(written, 10) - XCTAssertEqual(try Data(contentsOf: inputURL), Data(inputBytes)) + XCTAssertEqual(written, bytes.count, file: file, line: line) + XCTAssertEqual(try Data(contentsOf: inputURL), Data(bytes), file: file, line: line) default: - XCTFail() + XCTFail(file: file, line: line) + } + } + + func testThatConcurrentMultipleWritesDontProducePartialWrites() throws { + let inputURL = self.temporaryFileURL() + defer { self.removeFileIfExists(url: inputURL) } + + let stream = try FileOutputStream(url: inputURL, options: [.create]) + + /// Note: 10 iterations seems to be the amount of concurrent + /// runs Dispatch will give us so we limit it to that and + /// instead have each block write many times. + /// + DispatchQueue.concurrentPerform(iterations: 10) { (iteration) in + + for writeNumber in 0..<5000 { + + /// Random delay between writes + usleep(UInt32.random(in: 1...1000)) + + let iterationMessage = "Iteration \(iteration), write \(writeNumber)" + + let bytes = Array("\(iterationMessage).".utf8) + + switch stream.write(bytes) { + case .success(let written): + XCTAssertEqual(written, bytes.count, "\(iterationMessage): failed byte count test.") + default: + XCTFail("\(iterationMessage): failed.") + } + } } } - func testWriteToFileWithFailedWriteOnClosedFile() throws { + func testWriteThrowsOnAClosedFileDescriptor() throws { let inputBytes = Array(repeating: 128, count: 10) let inputURL = self.temporaryFileURL() @@ -129,6 +186,27 @@ class FileOutputStreamTests: XCTestCase { } } + func testWritePerformance() throws { + let inputURL = self.temporaryFileURL() + defer { self.removeFileIfExists(url: inputURL) } + + let inputBytes = Array(repeating: 128, count: 128) + + let stream = try FileOutputStream(url: inputURL, options: [.create]) + + self.measure { + for _ in 0..<100000 { + + switch stream.write(inputBytes) { + case .success(_): + break + default: + XCTFail() + } + } + } + } + // MARK: - Test helper functions private func temporaryFileURL() -> URL { diff --git a/Tests/TraceLogTests/Utilities/Streams/OutputStreamError+PosixTests.swift b/Tests/TraceLogTests/Utilities/Streams/OutputStreamError+PosixTests.swift index 0ea9699c..49a0f3b3 100644 --- a/Tests/TraceLogTests/Utilities/Streams/OutputStreamError+PosixTests.swift +++ b/Tests/TraceLogTests/Utilities/Streams/OutputStreamError+PosixTests.swift @@ -27,7 +27,6 @@ import XCTest import Glibc #endif - class OutputStreamErrorPosixTests: XCTestCase { func testErrorForOutOfRangeCode() { @@ -69,42 +68,6 @@ class OutputStreamErrorPosixTests: XCTestCase { else { XCTFail(".networkDown was expected"); return } } - func testErrorForEAGAIN() { - - /// Note: We rely on the strerror(errno) from the system - /// to provide the actual message so we ignore it in these - /// tests at the moment. IN the current implementation - /// of Swift the message is slightly different between - /// Darwin and Linux. - /// - guard case .interrupted(_) = OutputStreamError.error(for: EAGAIN) - else { XCTFail(".interrupted was expected"); return } - } - - func testErrorForEWOULDBLOCK() { - - /// Note: We rely on the strerror(errno) from the system - /// to provide the actual message so we ignore it in these - /// tests at the moment. IN the current implementation - /// of Swift the message is slightly different between - /// Darwin and Linux. - /// - guard case .interrupted(_) = OutputStreamError.error(for: EWOULDBLOCK) - else { XCTFail(".interrupted was expected"); return } - } - - func testErrorForEINTR() { - - /// Note: We rely on the strerror(errno) from the system - /// to provide the actual message so we ignore it in these - /// tests at the moment. IN the current implementation - /// of Swift the message is slightly different between - /// Darwin and Linux. - /// - guard case .interrupted(_) = OutputStreamError.error(for: EINTR) - else { XCTFail(".interrupted was expected"); return } - } - func testErrorForEBADF() { /// Note: We rely on the strerror(errno) from the system @@ -237,5 +200,49 @@ class OutputStreamErrorPosixTests: XCTestCase { else { XCTFail(".invalidArgument was expected"); return } } + func testErrorForEAGAIN() { + + /// Note: We rely on the strerror(errno) from the system + /// to provide the actual message so we ignore it in these + /// tests at the moment. IN the current implementation + /// of Swift the message is slightly different between + /// Darwin and Linux. + /// + if case .unknownError(let code, _) = OutputStreamError.error(for: EAGAIN) { + XCTAssertEqual(code, EAGAIN) + } else { + XCTFail(".unknownError was expected") + } + } + + func testErrorForEWOULDBLOCK() { + + /// Note: We rely on the strerror(errno) from the system + /// to provide the actual message so we ignore it in these + /// tests at the moment. IN the current implementation + /// of Swift the message is slightly different between + /// Darwin and Linux. + /// + if case .unknownError(let code, _) = OutputStreamError.error(for: EWOULDBLOCK) { + XCTAssertEqual(code, EWOULDBLOCK) + } else { + XCTFail(".unknownError was expected") + } + } + + func testErrorForEINTR() { + + /// Note: We rely on the strerror(errno) from the system + /// to provide the actual message so we ignore it in these + /// tests at the moment. IN the current implementation + /// of Swift the message is slightly different between + /// Darwin and Linux. + /// + if case .unknownError(let code, _) = OutputStreamError.error(for: EINTR) { + XCTAssertEqual(code, EINTR) + } else { + XCTFail(".unknownError was expected") + } + } } diff --git a/Tests/TraceLogTests/Utilities/Streams/OutputStreamError+PosixTests.swift.gyb b/Tests/TraceLogTests/Utilities/Streams/OutputStreamError+PosixTests.swift.gyb index 38de6e45..d7768360 100644 --- a/Tests/TraceLogTests/Utilities/Streams/OutputStreamError+PosixTests.swift.gyb +++ b/Tests/TraceLogTests/Utilities/Streams/OutputStreamError+PosixTests.swift.gyb @@ -32,10 +32,6 @@ import XCTest ["ENETDOWN", "networkDown", "Network is down"], ["ENETUNREACH", "networkDown", "Network is unreachable"], - ["EAGAIN", "interrupted", "Resource temporarily unavailable"], - ["EWOULDBLOCK", "interrupted", "Resource temporarily unavailable"], - ["EINTR", "interrupted", "Interrupted system call"], - ["EBADF", "disconnected", "Bad file descriptor"], ["EPIPE", "disconnected", "Broken pipe"], ["ECONNRESET", "disconnected", "Connection reset by peer"], @@ -51,8 +47,13 @@ import XCTest ["EINVAL", "invalidArgument", "Invalid argument"] ] -}% + TwoArgVariants = [ + ["EAGAIN", "unknownError", "Resource temporarily unavailable"], + ["EWOULDBLOCK", "unknownError", "Resource temporarily unavailable"], + ["EINTR", "unknownError", "Interrupted system call"] + ] +}% class OutputStreamErrorPosixTests: XCTestCase { func testErrorForOutOfRangeCode() { @@ -84,5 +85,21 @@ class OutputStreamErrorPosixTests: XCTestCase { } % end +% for (CodeConstant, CaseValue, Message) in TwoArgVariants: + func testErrorFor${CodeConstant}() { + + /// Note: We rely on the strerror(errno) from the system + /// to provide the actual message so we ignore it in these + /// tests at the moment. IN the current implementation + /// of Swift the message is slightly different between + /// Darwin and Linux. + /// + if case .${CaseValue}(let code, _) = OutputStreamError.error(for: ${CodeConstant}) { + XCTAssertEqual(code, ${CodeConstant}) + } else { + XCTFail(".${CaseValue} was expected") + } + } +% end } diff --git a/Tests/TraceLogTests/XCTestManifests.swift b/Tests/TraceLogTests/XCTestManifests.swift index d82b482c..0d317532 100644 --- a/Tests/TraceLogTests/XCTestManifests.swift +++ b/Tests/TraceLogTests/XCTestManifests.swift @@ -93,8 +93,13 @@ extension FileOutputStreamTests { ("testPositionReturnsZeroWhenGivenAnInvalidFD", testPositionReturnsZeroWhenGivenAnInvalidFD), ("testtestPositionReturnsZeroWhenAppliedToStandardError", testtestPositionReturnsZeroWhenAppliedToStandardError), ("testtestPositionReturnsZeroWhenAppliedToStandardOut", testtestPositionReturnsZeroWhenAppliedToStandardOut), + ("testThatConcurrentMultipleWritesDontProducePartialWrites", testThatConcurrentMultipleWritesDontProducePartialWrites), + ("testWritePerformance", testWritePerformance), + ("testWriteThrowsOnAClosedFileDescriptor", testWriteThrowsOnAClosedFileDescriptor), ("testWriteToFile", testWriteToFile), - ("testWriteToFileWithFailedWriteOnClosedFile", testWriteToFileWithFailedWriteOnClosedFile), + ("testWriteWithJustOverSystemPageSizes", testWriteWithJustOverSystemPageSizes), + ("testWriteWithLargeWrites", testWriteWithLargeWrites), + ("testWriteWithSystemPageSizes", testWriteWithSystemPageSizes), ] }