Make NIOFileDescriptor/FileRegion/IOData Sendable & soft-deprecated (#…

…2598)

Motivation:

`IOData` is a legacy but alas also core type that needs to be
`Sendable`. Before this PR however it can't be `Sendable` because it
holds a `FileRegion` which holds a `NIOFileDescriptor`. So let's make
all of these `Sendable` but let's also start the deprecation journey for
the following types:

- `IOData`, now soft-deprecated (no warnings) because on its reliance on
`FileRegion`
- `FileRegion`, now soft-deprecated (no warnings) because on its
reliance on `NIOFileHandle`
- `NIOFileHandle`, now soft-deprecated (warnings on the
`NIOFileHandle(descriptor:)` constructor but with a
`NIOFileHandle(_deprecatedTakingOwnershipOfDescriptor:)` alternative
- `NonBlockingFileIO`, now soft-deprecated (warnings on the `openFile`
functions (but with `_deprecated` alternatives) because of their
reliance on `NIOFileHandle)

Modification:

- Make `NIOFileDescriptor`, `FileRegion` and `IOData` `Sendable` by
tracking the fd number and the usage state in an atomic
- Enforce singular access by making the `withFileDescriptor { fd ... }`
function atomically exchange the fd number for a "I'm busy" sentinel
value
- Start deprecating `IOData`, `NIOFileHandle`, `NonBlockingFileIO`,
`FileRegion`

Result:
- `NIOFileDescriptor`, `FileRegion` and `IOData` can be `Sendable`

Sources/NIOCore/ChannelInvoker.swift

@@ -195,7 +195,7 @@ extension ChannelOutboundInvoker {
     public func close(mode: CloseMode = .all, file: StaticString = #fileID, line: UInt = #line) -> EventLoopFuture<Void>
     {
         let promise = makePromise(file: file, line: line)
-        close(mode: mode, promise: promise)
+        self.close(mode: mode, promise: promise)
         return promise.futureResult
     }
 

Sources/NIOCore/FileHandle.swift

@@ -2,7 +2,7 @@
 //
 // This source file is part of the SwiftNIO open source project
 //
-// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors
+// Copyright (c) 2017-2024 Apple Inc. and the SwiftNIO project authors
 // Licensed under Apache License v2.0
 //
 // See LICENSE.txt for license information
@@ -11,6 +11,9 @@
 // SPDX-License-Identifier: Apache-2.0
 //
 //===----------------------------------------------------------------------===//
+
+import Atomics
+
 #if os(Windows)
 import ucrt
 #elseif canImport(Darwin)
@@ -34,7 +37,105 @@ public typealias NIOPOSIXFileMode = CInt
 public typealias NIOPOSIXFileMode = mode_t
 #endif
 
-/// A `NIOFileHandle` is a handle to an open file.
+#if arch(x86_64) || arch(arm64)
+// 64 bit architectures
+typealias OneUInt32 = UInt32
+typealias TwoUInt32s = UInt64
+
+// Now we need to make `UInt64` match `DoubleWord`'s API but we can't use a custom
+// type because we need special support by the `swift-atomics` package.
+extension UInt64 {
+    fileprivate init(first: UInt32, second: UInt32) {
+        self = UInt64(first) << 32 | UInt64(second)
+    }
+
+    fileprivate var first: UInt32 {
+        get {
+            UInt32(truncatingIfNeeded: self >> 32)
+        }
+        set {
+            self = (UInt64(newValue) << 32) | UInt64(self.second)
+        }
+    }
+
+    fileprivate var second: UInt32 {
+        get {
+            UInt32(truncatingIfNeeded: self & 0xff_ff_ff_ff)
+        }
+        set {
+            self = (UInt64(self.first) << 32) | UInt64(newValue)
+        }
+    }
+}
+#elseif arch(arm) || arch(i386) || arch(arm64_32)
+// 32 bit architectures
+// Note: for testing purposes you can also use these defines for 64 bit platforms, they'll just consume twice as
+// much space, nothing else will go bad.
+typealias OneUInt32 = UInt
+typealias TwoUInt32s = DoubleWord
+#else
+#error("Unknown architecture")
+#endif
+
+internal struct FileDescriptorState {
+    private static let closedValue: OneUInt32 = 0xdead
+    private static let inUseValue: OneUInt32 = 0xbeef
+    private static let openValue: OneUInt32 = 0xcafe
+    internal var rawValue: TwoUInt32s
+
+    internal init(rawValue: TwoUInt32s) {
+        self.rawValue = rawValue
+    }
+
+    internal init(descriptor: CInt) {
+        self.rawValue = TwoUInt32s(
+            first: .init(truncatingIfNeeded: CUnsignedInt(bitPattern: descriptor)),
+            second: Self.openValue
+        )
+    }
+
+    internal var descriptor: CInt {
+        get {
+            CInt(bitPattern: UInt32(truncatingIfNeeded: self.rawValue.first))
+        }
+        set {
+            self.rawValue.first = .init(truncatingIfNeeded: CUnsignedInt(bitPattern: newValue))
+        }
+    }
+
+    internal var isOpen: Bool {
+        self.rawValue.second == Self.openValue
+    }
+
+    internal var isInUse: Bool {
+        self.rawValue.second == Self.inUseValue
+    }
+
+    internal var isClosed: Bool {
+        self.rawValue.second == Self.closedValue
+    }
+
+    mutating func close() {
+        assert(self.isOpen)
+        self.rawValue.second = Self.closedValue
+    }
+
+    mutating func markInUse() {
+        assert(self.isOpen)
+        self.rawValue.second = Self.inUseValue
+    }
+
+    mutating func markNotInUse() {
+        assert(self.rawValue.second == Self.inUseValue)
+        self.rawValue.second = Self.openValue
+    }
+}
+
+/// Deprecated. `NIOFileHandle` is a handle to an open file descriptor.
+///
+/// - warning: The `NIOFileHandle` API is deprecated, do not use going forward. It's not marked as `deprecated` yet such
+///            that users don't get the deprecation warnings affecting their APIs everywhere. For file I/O, please use
+///            the `NIOFileSystem` API.
 ///
 /// When creating a `NIOFileHandle` it takes ownership of the underlying file descriptor. When a `NIOFileHandle` is no longer
 /// needed you must `close` it or take back ownership of the file descriptor using `takeDescriptorOwnership`.
@@ -43,23 +144,62 @@ public typealias NIOPOSIXFileMode = mode_t
 ///
 /// - warning: Failing to manage the lifetime of a `NIOFileHandle` correctly will result in undefined behaviour.
 ///
-/// - warning: `NIOFileHandle` objects are not thread-safe and are mutable. They also cannot be fully thread-safe as they refer to a global underlying file descriptor.
-public final class NIOFileHandle: FileDescriptor {
-    public private(set) var isOpen: Bool
-    private let descriptor: CInt
+/// - Note: As of SwiftNIO 2.77.0, `NIOFileHandle` objects are are thread-safe and enforce singular access. If you access the same `NIOFileHandle`
+///         multiple times, it will throw `IOError(errorCode: EBUSY)` for the second access.
+public final class NIOFileHandle: FileDescriptor & Sendable {
+    private static let descriptorClosed: CInt = CInt.min
+    private let descriptor: UnsafeAtomic<TwoUInt32s>
+
+    public var isOpen: Bool {
+        FileDescriptorState(
+            rawValue: self.descriptor.load(ordering: .sequentiallyConsistent)
+        ).isOpen
+    }
+
+    private static func interpretDescriptorValueThrowIfInUseOrNotOpen(
+        _ descriptor: TwoUInt32s
+    ) throws -> FileDescriptorState {
+        let descriptorState = FileDescriptorState(rawValue: descriptor)
+        if descriptorState.isOpen {
+            return descriptorState
+        } else if descriptorState.isClosed {
+            throw IOError(errnoCode: EBADF, reason: "can't close file (as it's not open anymore).")
+        } else {
+            throw IOError(errnoCode: EBUSY, reason: "file descriptor currently in use")
+        }
+    }
+
+    private func peekAtDescriptorIfOpen() throws -> FileDescriptorState {
+        let descriptor = self.descriptor.load(ordering: .relaxed)
+        return try Self.interpretDescriptorValueThrowIfInUseOrNotOpen(descriptor)
+    }
+
+    /// Create a `NIOFileHandle` taking ownership of `descriptor`. You must call `NIOFileHandle.close` or `NIOFileHandle.takeDescriptorOwnership` before
+    /// this object can be safely released.
+    @available(
+        *,
+        deprecated,
+        message: """
+            Avoid using NIOFileHandle. The type is difficult to hold correctly, \
+            use NIOFileSystem as a replacement API.
+            """
+    )
+    public convenience init(descriptor: CInt) {
+        self.init(_deprecatedTakingOwnershipOfDescriptor: descriptor)
+    }
 
     /// Create a `NIOFileHandle` taking ownership of `descriptor`. You must call `NIOFileHandle.close` or `NIOFileHandle.takeDescriptorOwnership` before
     /// this object can be safely released.
-    public init(descriptor: CInt) {
-        self.descriptor = descriptor
-        self.isOpen = true
+    public init(_deprecatedTakingOwnershipOfDescriptor descriptor: CInt) {
+        self.descriptor = UnsafeAtomic.create(FileDescriptorState(descriptor: descriptor).rawValue)
     }
 
     deinit {
         assert(
             !self.isOpen,
             "leaked open NIOFileHandle(descriptor: \(self.descriptor)). Call `close()` to close or `takeDescriptorOwnership()` to take ownership and close by some other means."
         )
+        self.descriptor.destroy()
     }
 
     #if !os(WASI)
@@ -70,40 +210,85 @@ public final class NIOFileHandle: FileDescriptor {
     ///
     /// - Returns: A new `NIOFileHandle` with a fresh underlying file descriptor but shared seek pointer.
     public func duplicate() throws -> NIOFileHandle {
-        try withUnsafeFileDescriptor { fd in
-            NIOFileHandle(descriptor: try SystemCalls.dup(descriptor: fd))
+        try self.withUnsafeFileDescriptor { fd in
+            NIOFileHandle(_deprecatedTakingOwnershipOfDescriptor: try SystemCalls.dup(descriptor: fd))
         }
     }
     #endif
 
+    private func activateDescriptor(as descriptor: CInt) {
+        let desired = FileDescriptorState(descriptor: descriptor)
+        var expected = desired
+        expected.markInUse()
+        let (exchanged, original) = self.descriptor.compareExchange(
+            expected: expected.rawValue,
+            desired: desired.rawValue,
+            ordering: .sequentiallyConsistent
+        )
+        guard exchanged || FileDescriptorState(rawValue: original).isClosed else {
+            fatalError("bug in NIO (please report): NIOFileDescritor activate failed \(original)")
+        }
+    }
+
+    private func deactivateDescriptor(toClosed: Bool) throws -> CInt {
+        let peekedDescriptor = try self.peekAtDescriptorIfOpen()
+        // Don't worry, the above is just opportunistic. If we lose the race, we re-check below --> `!exchanged`
+        assert(peekedDescriptor.isOpen)
+        var desired = peekedDescriptor
+        if toClosed {
+            desired.close()
+        } else {
+            desired.markInUse()
+        }
+        assert(desired.rawValue != peekedDescriptor.rawValue, "\(desired.rawValue) == \(peekedDescriptor.rawValue)")
+        let (exchanged, originalDescriptor) = self.descriptor.compareExchange(
+            expected: peekedDescriptor.rawValue,
+            desired: desired.rawValue,
+            ordering: .sequentiallyConsistent
+        )
+
+        if exchanged {
+            assert(peekedDescriptor.rawValue == originalDescriptor)
+            return peekedDescriptor.descriptor
+        } else {
+            // We lost the race above, so this _will_ throw (as we're not closed).
+            let fauxDescriptor = try Self.interpretDescriptorValueThrowIfInUseOrNotOpen(originalDescriptor)
+            // This is impossible, because there are only 4 options in which the exchange above can fail
+            // 1. Descriptor already closed (would've thrown above)
+            // 2. Descriptor in use (would've thrown above)
+            // 3. Descriptor at illegal negative value (would've crashed above)
+            // 4. Descriptor a different, positive value (this is where we're at) --> memory corruption, let's crash
+            fatalError(
+                """
+                bug in NIO (please report): \
+                NIOFileDescriptor illegal state \
+                (\(peekedDescriptor), \(originalDescriptor), \(fauxDescriptor))")
+                """
+            )
+        }
+    }
+
     /// Take the ownership of the underlying file descriptor. This is similar to `close()` but the underlying file
     /// descriptor remains open. The caller is responsible for closing the file descriptor by some other means.
     ///
     /// After calling this, the `NIOFileHandle` cannot be used for anything else and all the operations will throw.
     ///
     /// - Returns: The underlying file descriptor, now owned by the caller.
     public func takeDescriptorOwnership() throws -> CInt {
-        guard self.isOpen else {
-            throw IOError(errnoCode: EBADF, reason: "can't close file (as it's not open anymore).")
-        }
-
-        self.isOpen = false
-        return self.descriptor
+        try self.deactivateDescriptor(toClosed: true)
     }
 
     public func close() throws {
-        try withUnsafeFileDescriptor { fd in
-            try SystemCalls.close(descriptor: fd)
-        }
-
-        self.isOpen = false
+        let descriptor = try self.deactivateDescriptor(toClosed: true)
+        try SystemCalls.close(descriptor: descriptor)
     }
 
     public func withUnsafeFileDescriptor<T>(_ body: (CInt) throws -> T) throws -> T {
-        guard self.isOpen else {
-            throw IOError(errnoCode: EBADF, reason: "file descriptor already closed!")
+        let descriptor = try self.deactivateDescriptor(toClosed: false)
+        defer {
+            self.activateDescriptor(as: descriptor)
         }
-        return try body(self.descriptor)
+        return try body(descriptor)
     }
 }
 
@@ -180,29 +365,75 @@ extension NIOFileHandle {
     ///   - path: The path of the file to open. The ownership of the file descriptor is transferred to this `NIOFileHandle` and so it will be closed once `close` is called.
     ///   - mode: Access mode. Default mode is `.read`.
     ///   - flags: Additional POSIX flags.
-    public convenience init(path: String, mode: Mode = .read, flags: Flags = .default) throws {
+    @available(
+        *,
+        deprecated,
+        message: """
+            Avoid using NIOFileHandle. The type is difficult to hold correctly, \
+            use NIOFileSystem as a replacement API.
+            """
+    )
+    @available(*, noasync, message: "This method may block the calling thread")
+    public convenience init(
+        path: String,
+        mode: Mode = .read,
+        flags: Flags = .default
+    ) throws {
+        try self.init(_deprecatedPath: path, mode: mode, flags: flags)
+    }
+
+    /// Open a new `NIOFileHandle`. This operation is blocking.
+    ///
+    /// - Parameters:
+    ///   - path: The path of the file to open. The ownership of the file descriptor is transferred to this `NIOFileHandle` and so it will be closed once `close` is called.
+    ///   - mode: Access mode. Default mode is `.read`.
+    ///   - flags: Additional POSIX flags.
+    @available(*, noasync, message: "This method may block the calling thread")
+    public convenience init(
+        _deprecatedPath path: String,
+        mode: Mode = .read,
+        flags: Flags = .default
+    ) throws {
         #if os(Windows)
         let fl = mode.posixFlags | flags.posixFlags | _O_NOINHERIT
         #else
         let fl = mode.posixFlags | flags.posixFlags | O_CLOEXEC
         #endif
         let fd = try SystemCalls.open(file: path, oFlag: fl, mode: flags.posixMode)
-        self.init(descriptor: fd)
+        self.init(_deprecatedTakingOwnershipOfDescriptor: fd)
     }
 
     /// Open a new `NIOFileHandle`. This operation is blocking.
     ///
     /// - Parameters:
     ///   - path: The path of the file to open. The ownership of the file descriptor is transferred to this `NIOFileHandle` and so it will be closed once `close` is called.
+    @available(
+        *,
+        deprecated,
+        message: """
+            Avoid using NIOFileHandle. The type is difficult to hold correctly, \
+            use NIOFileSystem as a replacement API.
+            """
+    )
+    @available(*, noasync, message: "This method may block the calling thread")
     public convenience init(path: String) throws {
+        try self.init(_deprecatedPath: path)
+    }
+
+    /// Open a new `NIOFileHandle`. This operation is blocking.
+    ///
+    /// - Parameters:
+    ///   - path: The path of the file to open. The ownership of the file descriptor is transferred to this `NIOFileHandle` and so it will be closed once `close` is called.
+    @available(*, noasync, message: "This method may block the calling thread")
+    public convenience init(_deprecatedPath path: String) throws {
         // This function is here because we had a function like this in NIO 2.0, and the one above doesn't quite match. Sadly we can't
         // really deprecate this either, because it'll be preferred to the one above in many cases.
-        try self.init(path: path, mode: .read, flags: .default)
+        try self.init(_deprecatedPath: path, mode: .read, flags: .default)
     }
 }
 
 extension NIOFileHandle: CustomStringConvertible {
     public var description: String {
-        "FileHandle { descriptor: \(self.descriptor) }"
+        "FileHandle { descriptor: \(FileDescriptorState(rawValue: self.descriptor.load(ordering: .relaxed)).descriptor) }"
     }
 }