Skip to content

Commit

Permalink
propagate socket errors on .reset
Browse files Browse the repository at this point in the history
Motivation:

Since apple#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
  • Loading branch information
weissi committed Apr 18, 2018
1 parent 5327cb1 commit 640cf11
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 7 deletions.
22 changes: 21 additions & 1 deletion Sources/NIO/BaseSocketChannel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,27 @@ class BaseSocketChannel<T: BaseSocket>: 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)
}

Expand Down
12 changes: 6 additions & 6 deletions Sources/NIO/IO.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<UInt8>(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)"
}
}

Expand All @@ -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")
}
}
}
Expand Down
1 change: 1 addition & 0 deletions Tests/NIOTests/ChannelTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ extension ChannelTests {
("testCloseSocketWhenReadErrorWasReceivedAndMakeSureNoReadCompleteArrives", testCloseSocketWhenReadErrorWasReceivedAndMakeSureNoReadCompleteArrives),
("testSocketFailingAsyncCorrectlyTearsTheChannelDownAndDoesntCrash", testSocketFailingAsyncCorrectlyTearsTheChannelDownAndDoesntCrash),
("testSocketErroringSynchronouslyCorrectlyTearsTheChannelDown", testSocketErroringSynchronouslyCorrectlyTearsTheChannelDown),
("testConnectWithECONNREFUSEDGetsTheRightError", testConnectWithECONNREFUSEDGetsTheRightError),
]
}
}
Expand Down
26 changes: 26 additions & 0 deletions Tests/NIOTests/ChannelTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 640cf11

Please sign in to comment.