Return failed EventLoopFuture when getOption(...) / setOption(...) is…

… called on a closed Channel

Motivation:

We should return a failed EventLoopFuture when getOption(...) / setOption(...) is called on a closed Channel as otherwise it may not be safe to modify the Channel ioptions after its closed.

Modifications:

- Add guards that check if the Channel is still open and if not fail the operation.
- Add testcase

Result:

Calling getOption(...) / setOption(...) on a closed Channel fails.

Sources/NIO/SocketChannel.swift

@@ -284,6 +284,10 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
     fileprivate func setOption0<T: ChannelOption>(option: T, value: T.OptionType) throws {
         assert(eventLoop.inEventLoop)
 
+        guard isOpen else {
+            throw ChannelError.alreadyClosed
+        }
+
         switch option {
         case _ as SocketOption:
             let (level, name) = option.value as! (SocketOptionLevel, SocketOptionName)
@@ -328,6 +332,10 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
     fileprivate func getOption0<T: ChannelOption>(option: T) throws -> T.OptionType {
         assert(eventLoop.inEventLoop)
 
+        guard isOpen else {
+            throw ChannelError.alreadyClosed
+        }
+
         switch option {
         case _ as SocketOption:
             let (level, name) = option.value as! (SocketOptionLevel, SocketOptionName)
@@ -802,6 +810,11 @@ final class SocketChannel: BaseSocketChannel<Socket> {
 
     override fileprivate func setOption0<T: ChannelOption>(option: T, value: T.OptionType) throws {
         assert(eventLoop.inEventLoop)
+
+        guard isOpen else {
+            throw ChannelError.alreadyClosed
+        }
+
         switch option {
         case _ as ConnectTimeoutOption:
             connectTimeout = value as? TimeAmount
@@ -818,6 +831,11 @@ final class SocketChannel: BaseSocketChannel<Socket> {
 
     override fileprivate func getOption0<T: ChannelOption>(option: T) throws -> T.OptionType {
         assert(eventLoop.inEventLoop)
+
+        guard isOpen else {
+            throw ChannelError.alreadyClosed
+        }
+
         switch option {
         case _ as ConnectTimeoutOption:
             return connectTimeout as! T.OptionType
@@ -1046,6 +1064,11 @@ final class ServerSocketChannel: BaseSocketChannel<ServerSocket> {
 
     override fileprivate func setOption0<T: ChannelOption>(option: T, value: T.OptionType) throws {
         assert(eventLoop.inEventLoop)
+
+        guard isOpen else {
+            throw ChannelError.alreadyClosed
+        }
+
         switch option {
         case _ as BacklogOption:
             backlog = value as! Int32
@@ -1056,6 +1079,11 @@ final class ServerSocketChannel: BaseSocketChannel<ServerSocket> {
 
     override fileprivate func getOption0<T: ChannelOption>(option: T) throws -> T.OptionType {
         assert(eventLoop.inEventLoop)
+
+        guard isOpen else {
+            throw ChannelError.alreadyClosed
+        }
+
         switch option {
         case _ as BacklogOption:
             return backlog as! T.OptionType
@@ -1212,6 +1240,11 @@ final class DatagramChannel: BaseSocketChannel<Socket> {
 
     override fileprivate func setOption0<T: ChannelOption>(option: T, value: T.OptionType) throws {
         assert(eventLoop.inEventLoop)
+
+        guard isOpen else {
+            throw ChannelError.alreadyClosed
+        }
+
         switch option {
         case _ as WriteSpinOption:
             pendingWrites.writeSpinCount = value as! UInt
@@ -1224,6 +1257,11 @@ final class DatagramChannel: BaseSocketChannel<Socket> {
 
     override fileprivate func getOption0<T: ChannelOption>(option: T) throws -> T.OptionType {
         assert(eventLoop.inEventLoop)
+
+        guard isOpen else {
+            throw ChannelError.alreadyClosed
+        }
+
         switch option {
         case _ as WriteSpinOption:
             return pendingWrites.writeSpinCount as! T.OptionType

Tests/NIOTests/SocketChannelTest+XCTest.swift

@@ -34,6 +34,7 @@ extension SocketChannelTest {
                 ("testAcceptFailsWithENOBUFS", testAcceptFailsWithENOBUFS),
                 ("testAcceptFailsWithENOMEM", testAcceptFailsWithENOMEM),
                 ("testAcceptFailsWithEFAULT", testAcceptFailsWithEFAULT),
+                ("testSetGetOptionClosedServerSocketChannel", testSetGetOptionClosedServerSocketChannel),
            ]
    }
 }

Tests/NIOTests/SocketChannelTest.swift

@@ -164,4 +164,36 @@ public class SocketChannelTest : XCTestCase {
         let ioError = try promise.futureResult.wait()
         XCTAssertEqual(error, ioError.errnoCode)
     }
+
+    public func testSetGetOptionClosedServerSocketChannel() throws {
+        let group = MultiThreadedEventLoopGroup(numThreads: 1)
+        defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) }
+
+        // Create two channels with different event loops.
+        let serverChannel = try ServerBootstrap(group: group).bind(host: "127.0.0.1", port: 0).wait()
+        let clientChannel = try ClientBootstrap(group: group).connect(to: serverChannel.localAddress!).wait()
+
+        try assertSetGetOptionOnOpenAndClosed(channel: clientChannel, option: ChannelOptions.allowRemoteHalfClosure, value: true)
+        try assertSetGetOptionOnOpenAndClosed(channel: serverChannel, option: ChannelOptions.backlog, value: 100)
+
+    }
+
+    private func assertSetGetOptionOnOpenAndClosed<T: ChannelOption>(channel: Channel, option: T, value: T.OptionType) throws {
+        _ = try channel.setOption(option: option, value: value).wait()
+        _ = try channel.getOption(option: option).wait()
+        try channel.close().wait()
+        try channel.closeFuture.wait()
+
+        do {
+            _ = try channel.setOption(option: option, value: value).wait()
+        } catch let err as ChannelError where err == .alreadyClosed {
+            // expected
+        }
+
+        do {
+            _ = try channel.getOption(option: option).wait()
+        } catch let err as ChannelError where err == .alreadyClosed {
+            // expected
+        }
+    }
 }