Skip to content

Commit cc1e527

Browse files
authored
Make SynchronizedFileSink.close unavailable from async (#195)
Motivation syncClose will block whatever thread it's on indefinitely. That makes it unsafe to call in async contexts. Modifications Add a new close() method that's async. Make the existing method unavailable from async. Add some tests. Results Easier to close these from async contexts
1 parent d75ed70 commit cc1e527

File tree

4 files changed

+191
-3
lines changed

4 files changed

+191
-3
lines changed

Sources/NIOExtras/WritePCAPHandler.swift

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -599,7 +599,7 @@ extension NIOWritePCAPHandler {
599599
/// A synchronised file sink that uses a `DispatchQueue` to do all the necessary write synchronously.
600600
///
601601
/// A `SynchronizedFileSink` is thread-safe so can be used from any thread/`EventLoop`. After use, you
602-
/// _must_ call `syncClose` on the `SynchronizedFileSink` to shut it and all the associated resources down. Failing
602+
/// _must_ call `syncClose` or `close` on the `SynchronizedFileSink` to shut it and all the associated resources down. Failing
603603
/// to do so triggers undefined behaviour.
604604
public final class SynchronizedFileSink {
605605
private let fileHandle: NIOFileHandle
@@ -682,17 +682,44 @@ extension NIOWritePCAPHandler {
682682
self.workQueue = DispatchQueue(label: "io.swiftnio.extras.WritePCAPHandler.SynchronizedFileSink.workQueue")
683683
self.errorHandler = errorHandler
684684
}
685-
685+
686+
#if swift(>=5.7)
687+
/// Synchronously close this `SynchronizedFileSink` and any associated resources.
688+
///
689+
/// After use, it is mandatory to close a `SynchronizedFileSink` exactly once. `syncClose` may be called from
690+
/// any thread but not from an `EventLoop` as it will block, and may not be called from an async context.
691+
@available(*, noasync, message: "syncClose() can block indefinitely, prefer close()", renamed: "close()")
692+
public func syncClose() throws {
693+
try self._syncClose()
694+
}
695+
#else
686696
/// Synchronously close this `SynchronizedFileSink` and any associated resources.
687697
///
688698
/// After use, it is mandatory to close a `SynchronizedFileSink` exactly once. `syncClose` may be called from
689-
/// any thread but not from an `EventLoop` as it will block.
699+
/// any thread but not from an `EventLoop` as it will block, and may not be called from an async context.
690700
public func syncClose() throws {
701+
try self._syncClose()
702+
}
703+
#endif
704+
705+
private func _syncClose() throws {
691706
self.writesGroup.wait()
692707
try self.workQueue.sync {
693708
try self.fileHandle.close()
694709
}
695710
}
711+
712+
/// Close this `SynchronizedFileSink` and any associated resources.
713+
///
714+
/// After use, it is mandatory to close a `SynchronizedFileSink` exactly once.
715+
@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
716+
public func close() async throws {
717+
try await withCheckedThrowingContinuation { continuation in
718+
self.workQueue.async {
719+
continuation.resume(with: Result { try self.fileHandle.close() })
720+
}
721+
}
722+
}
696723

697724
public func write(buffer: ByteBuffer) {
698725
self.workQueue.async(group: self.writesGroup) {

Tests/LinuxMain.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ class LinuxMainRunner {
5959
testCase(ServerResponseTests.allTests),
6060
testCase(ServerStateMachineTests.allTests),
6161
testCase(SocksClientHandlerTests.allTests),
62+
testCase(SynchronizedFileSinkTests.allTests),
6263
testCase(WritePCAPHandlerTest.allTests),
6364
])
6465
}
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the SwiftNIO open source project
4+
//
5+
// Copyright (c) 2018-2023 Apple Inc. and the SwiftNIO project authors
6+
// Licensed under Apache License v2.0
7+
//
8+
// See LICENSE.txt for license information
9+
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
10+
//
11+
// SPDX-License-Identifier: Apache-2.0
12+
//
13+
//===----------------------------------------------------------------------===//
14+
//
15+
// SynchronizedFileSinkTests+XCTest.swift
16+
//
17+
import XCTest
18+
19+
///
20+
/// NOTE: This file was generated by generate_linux_tests.rb
21+
///
22+
/// Do NOT edit this file directly as it will be regenerated automatically when needed.
23+
///
24+
25+
extension SynchronizedFileSinkTests {
26+
27+
@available(*, deprecated, message: "not actually deprecated. Just deprecated to allow deprecated tests (which test deprecated functionality) without warnings")
28+
static var allTests : [(String, (SynchronizedFileSinkTests) -> () throws -> Void)] {
29+
return [
30+
("testSimpleFileSink", testSimpleFileSink),
31+
("testSimpleFileSinkAsyncShutdown", testSimpleFileSinkAsyncShutdown),
32+
]
33+
}
34+
}
35+
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the SwiftNIO open source project
4+
//
5+
// Copyright (c) 2023 Apple Inc. and the SwiftNIO project authors
6+
// Licensed under Apache License v2.0
7+
//
8+
// See LICENSE.txt for license information
9+
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
10+
//
11+
// SPDX-License-Identifier: Apache-2.0
12+
//
13+
//===----------------------------------------------------------------------===//
14+
15+
import Foundation
16+
import XCTest
17+
18+
import NIOCore
19+
import NIOEmbedded
20+
@testable import NIOExtras
21+
22+
final class SynchronizedFileSinkTests: XCTestCase {
23+
func testSimpleFileSink() throws {
24+
try withTemporaryFile { file, path in
25+
let sink = try NIOWritePCAPHandler.SynchronizedFileSink.fileSinkWritingToFile(path: path, errorHandler: { XCTFail("Caught error \($0)") })
26+
27+
sink.write(buffer: ByteBuffer(string: "Hello, "))
28+
sink.write(buffer: ByteBuffer(string: "world!"))
29+
try sink.syncClose()
30+
31+
let data = try Data(contentsOf: URL(fileURLWithPath: path))
32+
XCTAssertEqual(data, Data(NIOWritePCAPHandler.pcapFileHeader.readableBytesView) + Data("Hello, world!".utf8))
33+
}
34+
}
35+
36+
func testSimpleFileSinkAsyncShutdown() throws {
37+
guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return }
38+
XCTAsyncTest {
39+
try await withTemporaryFile { file, path in
40+
let sink = try NIOWritePCAPHandler.SynchronizedFileSink.fileSinkWritingToFile(path: path, errorHandler: { XCTFail("Caught error \($0)") })
41+
42+
sink.write(buffer: ByteBuffer(string: "Hello, "))
43+
sink.write(buffer: ByteBuffer(string: "world!"))
44+
try await sink.close()
45+
46+
let data = try Data(contentsOf: URL(fileURLWithPath: path))
47+
XCTAssertEqual(data, Data(NIOWritePCAPHandler.pcapFileHeader.readableBytesView) + Data("Hello, world!".utf8))
48+
}
49+
}
50+
}
51+
}
52+
53+
fileprivate func withTemporaryFile<T>(content: String? = nil, _ body: (NIOCore.NIOFileHandle, String) throws -> T) throws -> T {
54+
let temporaryFilePath = "\(temporaryDirectory)/nio_extras_\(UUID())"
55+
FileManager.default.createFile(atPath: temporaryFilePath, contents: content?.data(using: .utf8))
56+
defer {
57+
XCTAssertNoThrow(try FileManager.default.removeItem(atPath: temporaryFilePath))
58+
}
59+
60+
let fileHandle = try NIOFileHandle(path: temporaryFilePath, mode: [.read, .write])
61+
defer {
62+
XCTAssertNoThrow(try fileHandle.close())
63+
}
64+
65+
return try body(fileHandle, temporaryFilePath)
66+
}
67+
68+
fileprivate func withTemporaryFile<T>(content: String? = nil, _ body: (NIOCore.NIOFileHandle, String) async throws -> T) async throws -> T {
69+
let temporaryFilePath = "\(temporaryDirectory)/nio_extras_\(UUID())"
70+
FileManager.default.createFile(atPath: temporaryFilePath, contents: content?.data(using: .utf8))
71+
defer {
72+
XCTAssertNoThrow(try FileManager.default.removeItem(atPath: temporaryFilePath))
73+
}
74+
75+
let fileHandle = try NIOFileHandle(path: temporaryFilePath, mode: [.read, .write])
76+
defer {
77+
XCTAssertNoThrow(try fileHandle.close())
78+
}
79+
80+
return try await body(fileHandle, temporaryFilePath)
81+
}
82+
83+
fileprivate var temporaryDirectory: String {
84+
#if os(Linux)
85+
return "/tmp"
86+
#else
87+
if #available(macOS 10.12, iOS 10, tvOS 10, watchOS 3, *) {
88+
return FileManager.default.temporaryDirectory.path
89+
} else {
90+
return "/tmp"
91+
}
92+
#endif // os
93+
}
94+
95+
extension XCTestCase {
96+
@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
97+
/// Cross-platform XCTest support for async-await tests.
98+
///
99+
/// Currently the Linux implementation of XCTest doesn't have async-await support.
100+
/// Until it does, we make use of this shim which uses a detached `Task` along with
101+
/// `XCTest.wait(for:timeout:)` to wrap the operation.
102+
///
103+
/// - NOTE: Support for Linux is tracked by https://bugs.swift.org/browse/SR-14403.
104+
/// - NOTE: Implementation currently in progress: https://github.com/apple/swift-corelibs-xctest/pull/326
105+
func XCTAsyncTest(
106+
expectationDescription: String = "Async operation",
107+
timeout: TimeInterval = 30,
108+
file: StaticString = #filePath,
109+
line: UInt = #line,
110+
function: StaticString = #function,
111+
operation: @escaping @Sendable () async throws -> Void
112+
) {
113+
let expectation = self.expectation(description: expectationDescription)
114+
Task {
115+
do {
116+
try await operation()
117+
} catch {
118+
XCTFail("Error thrown while executing \(function): \(error)", file: file, line: line)
119+
Thread.callStackSymbols.forEach { print($0) }
120+
}
121+
expectation.fulfill()
122+
}
123+
self.wait(for: [expectation], timeout: timeout)
124+
}
125+
}

0 commit comments

Comments
 (0)