From 640cf11c285456c1630dc55566f6eb13ad11e70f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Wei=C3=9F?= Date: Tue, 17 Apr 2018 19:06:23 +0100 Subject: [PATCH] propagate socket errors on .reset Motivation: Since #286, we do eagerly detect connection resets which is good. To resolve the situation we first try to read everything off the socket. If something fails there, everything is good and the client gets the correct error. If that however doesn't lead to any errors, we would close the connection with `error: ChannelError.eof` which the client would then see for example on a connect that happened to get `ECONNRESET` or `ECONNREFUSED`. That's not right so these situations are fixed here. Modifications: - on reset, try to get the socket error if any - if no socket error was detected, we send a `ECONNRESET` because we know we have been reset but not why and didn't encounter any errors - write tests for these situations Result: connection resets that don't hit any other errors will now see the correct error --- Sources/NIO/BaseSocketChannel.swift | 22 +++++++++++++++++++- Sources/NIO/IO.swift | 12 +++++------ Tests/NIOTests/ChannelTests+XCTest.swift | 1 + Tests/NIOTests/ChannelTests.swift | 26 ++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 7 deletions(-) diff --git a/Sources/NIO/BaseSocketChannel.swift b/Sources/NIO/BaseSocketChannel.swift index 066e3c133f..576ebd94ac 100644 --- a/Sources/NIO/BaseSocketChannel.swift +++ b/Sources/NIO/BaseSocketChannel.swift @@ -798,7 +798,27 @@ class BaseSocketChannel: SelectableChannel, ChannelCore { // the `reset` event. final func reset() { self.readEOF() - self.close0(error: ChannelError.eof, mode: .all, promise: nil) + + if self.socket.isOpen { + assert(self.lifecycleManager.isRegistered) + let error: IOError + // if the socket is still registered (and therefore open), let's try to get the actual socket error from the socket + do { + let result: Int32 = try self.socket.getOption(level: SOL_SOCKET, name: SO_ERROR) + if result != 0 { + // we have a socket error, let's forward + // this path will be executed on Linux (EPOLLERR) & Darwin (ev.fflags != 0) + error = IOError(errnoCode: result, reason: "connection reset (error set)") + } else { + // we don't have a socket error, this must be connection reset without an error then + // this path should only be executed on Linux (EPOLLHUP, no EPOLLERR) + error = IOError(errnoCode: ECONNRESET, reason: "connection reset (no error set)") + } + self.close0(error: error, mode: .all, promise: nil) + } catch { + self.close0(error: error, mode: .all, promise: nil) + } + } assert(!self.lifecycleManager.isRegistered) } diff --git a/Sources/NIO/IO.swift b/Sources/NIO/IO.swift index d3a2b8a1d1..36edadcb4b 100644 --- a/Sources/NIO/IO.swift +++ b/Sources/NIO/IO.swift @@ -53,17 +53,17 @@ public struct IOError: Swift.Error { /// /// - parameters: /// - errorCode: the `errno` that was set for the operation. -/// - function: the function / syscall that caused the error. +/// - reason: what failed /// -returns: the constructed reason. -private func reasonForError(errnoCode: Int32, function: StaticString) -> String { +private func reasonForError(errnoCode: Int32, reason: String) -> String { if let errorDescC = strerror(errnoCode) { let errorDescLen = strlen(errorDescC) return errorDescC.withMemoryRebound(to: UInt8.self, capacity: errorDescLen) { ptr in let errorDescPtr = UnsafeBufferPointer(start: ptr, count: errorDescLen) - return "\(function) failed: \(String(decoding: errorDescPtr, as: UTF8.self)) (errno: \(errnoCode)) " + return "\(reason): \(String(decoding: errorDescPtr, as: UTF8.self)) (errno: \(errnoCode)) " } } else { - return "\(function) failed: Broken strerror, unknown error: \(errnoCode)" + return "\(reason): Broken strerror, unknown error: \(errnoCode)" } } @@ -75,9 +75,9 @@ extension IOError: CustomStringConvertible { public var localizedDescription: String { switch self.reason { case .reason(let reason): - return reason + return reasonForError(errnoCode: self.errnoCode, reason: reason) case .function(let function): - return reasonForError(errnoCode: self.errnoCode, function: function) + return reasonForError(errnoCode: self.errnoCode, reason: "\(function) failed") } } } diff --git a/Tests/NIOTests/ChannelTests+XCTest.swift b/Tests/NIOTests/ChannelTests+XCTest.swift index 439f19a724..beaab11a44 100644 --- a/Tests/NIOTests/ChannelTests+XCTest.swift +++ b/Tests/NIOTests/ChannelTests+XCTest.swift @@ -67,6 +67,7 @@ extension ChannelTests { ("testCloseSocketWhenReadErrorWasReceivedAndMakeSureNoReadCompleteArrives", testCloseSocketWhenReadErrorWasReceivedAndMakeSureNoReadCompleteArrives), ("testSocketFailingAsyncCorrectlyTearsTheChannelDownAndDoesntCrash", testSocketFailingAsyncCorrectlyTearsTheChannelDownAndDoesntCrash), ("testSocketErroringSynchronouslyCorrectlyTearsTheChannelDown", testSocketErroringSynchronouslyCorrectlyTearsTheChannelDown), + ("testConnectWithECONNREFUSEDGetsTheRightError", testConnectWithECONNREFUSEDGetsTheRightError), ] } } diff --git a/Tests/NIOTests/ChannelTests.swift b/Tests/NIOTests/ChannelTests.swift index 72fb0350c3..38250f4805 100644 --- a/Tests/NIOTests/ChannelTests.swift +++ b/Tests/NIOTests/ChannelTests.swift @@ -2214,6 +2214,32 @@ public class ChannelTests: XCTestCase { XCTAssertNoThrow(try sc.closeFuture.wait()) XCTAssertNoThrow(try sc.syncCloseAcceptingAlreadyClosed()) } + + func testConnectWithECONNREFUSEDGetsTheRightError() throws { + let group = MultiThreadedEventLoopGroup(numThreads: 1) + defer { + XCTAssertNoThrow(try group.syncShutdownGracefully()) + } + let serverSock = try Socket(protocolFamily: PF_INET, type: Posix.SOCK_STREAM) + // we deliberately don't set SO_REUSEADDR + XCTAssertNoThrow(try serverSock.bind(to: SocketAddress(ipAddress: "127.0.0.1", port: 0))) + let serverSockAddress = try! serverSock.localAddress() + XCTAssertNoThrow(try serverSock.close()) + + // we're just looping here to get a pretty good chance we're hitting both the synchronous and the asynchronous + // connect path. + for _ in 0..<64 { + do { + _ = try ClientBootstrap(group: group).connect(to: serverSockAddress).wait() + XCTFail("just worked") + } catch let error as IOError where error.errnoCode == ECONNREFUSED { + // OK + } catch { + XCTFail("unexpected error: \(error)") + } + } + } + } fileprivate class VerifyConnectionFailureHandler: ChannelInboundHandler {