make isOpen a computed property (apple#123)

Motivation:

Reduce memory footprint of BaseSocket and FileHandle

Modifications:

- made isOpen a computed var from 'descriptor >= 0'
- add withSocketpair to TestUtils
- use real pipe and socketpair file descriptors for tests
- BaseSocket and FileHandle init check precondition 'descriptor >= 0'

Result:

- isOpen no longer requires storage space
- close() sets descriptor to -1
- descriptor property changed from let to var
- tests will need to use valid file descriptors

Sources/NIO/BaseSocket.swift

@@ -204,8 +204,10 @@ extension sockaddr_storage {
 /// This should not be created directly but one of its sub-classes should be used, like `ServerSocket` or `Socket`.
 class BaseSocket: Selectable {
 
-    private let descriptor: CInt
-    public private(set) var isOpen: Bool
+    private var descriptor: CInt
+    public var isOpen: Bool {
+        return descriptor >= 0
+    }
 
     func withUnsafeFileDescriptor<T>(_ body: (CInt) throws -> T) throws -> T {
         guard self.isOpen else {
@@ -283,8 +285,8 @@ class BaseSocket: Selectable {
     /// - parameters:
     ///     - descriptor: The file descriptor to wrap.
     init(descriptor: CInt) {
+        precondition(descriptor >= 0, "invalid file descriptor")
         self.descriptor = descriptor
-        self.isOpen = true
     }
 
     deinit {
@@ -381,7 +383,7 @@ class BaseSocket: Selectable {
             try Posix.close(descriptor: fd)
         }
 
-        self.isOpen = false
+        self.descriptor = -1
     }
 }
 

Sources/NIO/FileHandle.swift

@@ -23,14 +23,17 @@
 ///
 /// - warning: `FileHandle` 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 FileHandle: FileDescriptor {
-    public private(set) var isOpen: Bool
-    private let descriptor: CInt
+    private var descriptor: CInt
+
+    public var isOpen: Bool {
+        return descriptor >= 0
+    }
 
     /// Create a `FileHandle` taking ownership of `descriptor`. You must call `FileHandle.close` or `FileHandle.takeDescriptorOwnership` before
     /// this object can be safely released.
     public init(descriptor: CInt) {
+        precondition(descriptor >= 0, "invalid file descriptor")
         self.descriptor = descriptor
-        self.isOpen = true
     }
 
     deinit {
@@ -60,16 +63,16 @@ public final class FileHandle: FileDescriptor {
             throw IOError(errnoCode: EBADF, reason: "can't close file (as it's not open anymore).")
         }
 
-        self.isOpen = false
-        return self.descriptor
+        let savedDescriptor = self.descriptor
+        self.descriptor = -1
+        return savedDescriptor
     }
 
     public func close() throws {
         try withUnsafeFileDescriptor { fd in
             try Posix.close(descriptor: fd)
         }
-
-        self.isOpen = false
+        self.descriptor = -1
     }
 
     public func withUnsafeFileDescriptor<T>(_ body: (CInt) throws -> T) throws -> T {

Tests/NIOTests/BaseObjectsTest.swift

@@ -50,41 +50,37 @@ class BaseObjectTest: XCTestCase {
         }
     }
 
-    func testNIOFileRegionConversion() {
-        let handle = FileHandle(descriptor: -1)
-        let expected = FileRegion(fileHandle: handle, readerIndex: 1, endIndex: 2)
-        defer {
-            // fake descriptor, so shouldn't be closed.
-            XCTAssertNoThrow(try handle.takeDescriptorOwnership())
-        }
-        let asAny = NIOAny(expected)
-        XCTAssert(expected == asAny.forceAs(type: FileRegion.self))
-        XCTAssert(expected == asAny.forceAsFileRegion())
-        if let actual = asAny.tryAs(type: FileRegion.self) {
-            XCTAssert(expected == actual)
-        } else {
-            XCTFail("tryAs didn't work")
-        }
-        if let actual = asAny.tryAsFileRegion() {
-            XCTAssert(expected == actual)
-        } else {
-            XCTFail("tryAs didn't work")
+    func testNIOFileRegionConversion() throws {
+        try withPipe { (readFH, writeFH) in
+            let expected = FileRegion(fileHandle: readFH, readerIndex: 1, endIndex: 2)
+            let asAny = NIOAny(expected)
+            XCTAssert(expected == asAny.forceAs(type: FileRegion.self))
+            XCTAssert(expected == asAny.forceAsFileRegion())
+            if let actual = asAny.tryAs(type: FileRegion.self) {
+                XCTAssert(expected == actual)
+            } else {
+                XCTFail("tryAs didn't work")
+            }
+            if let actual = asAny.tryAsFileRegion() {
+                XCTAssert(expected == actual)
+            } else {
+                XCTFail("tryAs didn't work")
+            }
+            return [readFH, writeFH]
         }
     }
 
-    func testBadConversions() {
-        let handle = FileHandle(descriptor: -1)
-        let bb = ByteBufferAllocator().buffer(capacity: 1024)
-        let fr = FileRegion(fileHandle: handle, readerIndex: 1, endIndex: 2)
-        defer {
-            // fake descriptor, so shouldn't be closed.
-            XCTAssertNoThrow(try handle.takeDescriptorOwnership())
-        }
-        let id = IOData.byteBuffer(bb)
+    func testBadConversions() throws {
+        try withPipe { (readFH, writeFH) in
+            let bb = ByteBufferAllocator().buffer(capacity: 1024)
+            let fr = FileRegion(fileHandle: readFH, readerIndex: 1, endIndex: 2)
+            let id = IOData.byteBuffer(bb)
 
-        XCTAssertNil(NIOAny(bb).tryAsFileRegion())
-        XCTAssertNil(NIOAny(fr).tryAsByteBuffer())
-        XCTAssertNil(NIOAny(id).tryAsFileRegion())
+            XCTAssertNil(NIOAny(bb).tryAsFileRegion())
+            XCTAssertNil(NIOAny(fr).tryAsByteBuffer())
+            XCTAssertNil(NIOAny(id).tryAsFileRegion())
+            return [readFH, writeFH]
+        }
     }
 
     func testByteBufferFromIOData() {
@@ -93,29 +89,25 @@ class BaseObjectTest: XCTestCase {
         XCTAssertEqual(expected, NIOAny(wrapped).tryAsByteBuffer())
     }
 
-    func testFileRegionFromIOData() {
-        let handle = FileHandle(descriptor: -1)
-        let expected = FileRegion(fileHandle: handle, readerIndex: 1, endIndex: 2)
-        defer {
-            // fake descriptor, so shouldn't be closed.
-            XCTAssertNoThrow(try handle.takeDescriptorOwnership())
+    func testFileRegionFromIOData() throws {
+        try withPipe { (readFH, writeFH) in
+            let expected = FileRegion(fileHandle: readFH, readerIndex: 1, endIndex: 2)
+            let wrapped = IOData.fileRegion(expected)
+            XCTAssert(expected == NIOAny(wrapped).tryAsFileRegion())
+            return [readFH, writeFH]
         }
-        let wrapped = IOData.fileRegion(expected)
-        XCTAssert(expected == NIOAny(wrapped).tryAsFileRegion())
     }
 
-    func testIODataEquals() {
-        let handle = FileHandle(descriptor: -1)
-        var bb1 = ByteBufferAllocator().buffer(capacity: 1024)
-        let bb2 = ByteBufferAllocator().buffer(capacity: 1024)
-        bb1.write(string: "hello")
-        let fr = FileRegion(fileHandle: handle, readerIndex: 1, endIndex: 2)
-        defer {
-            // fake descriptor, so shouldn't be closed.
-            XCTAssertNoThrow(try handle.takeDescriptorOwnership())
+    func testIODataEquals() throws {
+        try withPipe { (readFH, writeFH) in
+            var bb1 = ByteBufferAllocator().buffer(capacity: 1024)
+            let bb2 = ByteBufferAllocator().buffer(capacity: 1024)
+            bb1.write(string: "hello")
+            let fr = FileRegion(fileHandle: readFH, readerIndex: 1, endIndex: 2)
+            XCTAssertEqual(IOData.byteBuffer(bb1), IOData.byteBuffer(bb1))
+            XCTAssertNotEqual(IOData.byteBuffer(bb1), IOData.byteBuffer(bb2))
+            XCTAssertNotEqual(IOData.byteBuffer(bb1), IOData.fileRegion(fr))
+            return [readFH, writeFH]
         }
-        XCTAssertEqual(IOData.byteBuffer(bb1), IOData.byteBuffer(bb1))
-        XCTAssertNotEqual(IOData.byteBuffer(bb1), IOData.byteBuffer(bb2))
-        XCTAssertNotEqual(IOData.byteBuffer(bb1), IOData.fileRegion(fr))
     }
 }

Tests/NIOTests/ChannelPipelineTest.swift

@@ -165,15 +165,13 @@ class ChannelPipelineTest: XCTestCase {
 
         XCTAssertTrue(loop.inEventLoop)
         do {
-            let handle = FileHandle(descriptor: -1)
-            let fr = FileRegion(fileHandle: handle, readerIndex: 0, endIndex: 0)
-            defer {
-                // fake descriptor, so shouldn't be closed.
-                XCTAssertNoThrow(try handle.takeDescriptorOwnership())
+            try withPipe { (readFH, writeFH) in
+                let fr = FileRegion(fileHandle: writeFH, readerIndex: 0, endIndex: 0)
+                try channel.writeOutbound(fr)
+                loop.run()
+                XCTFail("we ran but an error should have been thrown")
+                return [readFH, writeFH]
             }
-            try channel.writeOutbound(fr)
-            loop.run()
-            XCTFail("we ran but an error should have been thrown")
         } catch let err as ChannelError {
             XCTAssertEqual(err, .ioOnClosedChannel)
         }