Skip to content

Add defensive connection closure. #328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Sources/AsyncHTTPClient/Connection.swift
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ extension Connection {
func close() -> EventLoopFuture<Void> {
return self.channel.close()
}

func close(promise: EventLoopPromise<Void>?) {
return self.channel.close(promise: promise)
}
}

/// Methods of Connection which are used in ConnectionsState extracted as protocol
Expand Down
8 changes: 8 additions & 0 deletions Sources/AsyncHTTPClient/ConnectionPool.swift
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,14 @@ class HTTP1ConnectionProvider {
self.state.release(connection: connection, closing: closing)
}

// We close defensively here: we may have failed to actually close on other codepaths,
// or we may be expecting the server to close. In either case, we want our FD back, so
// we close now to cover our backs. We don't care about the result: if the channel is
// _already_ closed, that's fine by us.
if closing {
connection.close(promise: nil)
}

switch action {
case .none:
break
Expand Down
2 changes: 1 addition & 1 deletion Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,7 @@ class HTTPClientInternalTests: XCTestCase {

XCTAssert(connection !== connection2)
try! connection2.channel.eventLoop.submit {
connection2.release(closing: true, logger: HTTPClient.loggingDisabled)
connection2.release(closing: false, logger: HTTPClient.loggingDisabled)
}.wait()
XCTAssertTrue(connection2.channel.isActive)
}
Expand Down
47 changes: 47 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,53 @@ internal final class HttpBinForSSLUncleanShutdownHandler: ChannelInboundHandler
}
}

internal final class CloseWithoutClosingServerHandler: ChannelInboundHandler {
typealias InboundIn = HTTPServerRequestPart
typealias OutboundOut = HTTPServerResponsePart

private var callback: (() -> Void)?
private var onClosePromise: EventLoopPromise<Void>?

init(_ callback: @escaping () -> Void) {
self.callback = callback
}

func handlerAdded(context: ChannelHandlerContext) {
self.onClosePromise = context.eventLoop.makePromise()
self.onClosePromise!.futureResult.whenSuccess(self.callback!)
self.callback = nil
}

func handlerRemoved(context: ChannelHandlerContext) {
assert(self.onClosePromise == nil)
}

func channelInactive(context: ChannelHandlerContext) {
if let onClosePromise = self.onClosePromise {
self.onClosePromise = nil
onClosePromise.succeed(())
}
}

func channelRead(context: ChannelHandlerContext, data: NIOAny) {
guard case .end = self.unwrapInboundIn(data) else {
return
}

// We're gonna send a response back here, with Connection: close, but we will
// not close the connection. This reproduces #324.
let headers = HTTPHeaders([
("Host", "CloseWithoutClosingServerHandler"),
("Content-Length", "0"),
("Connection", "close"),
])
let head = HTTPResponseHead(version: .init(major: 1, minor: 1), status: .ok, headers: headers)

context.write(self.wrapOutboundOut(.head(head)), promise: nil)
context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
}
}

struct EventLoopFutureTimeoutError: Error {}

extension EventLoopFuture {
Expand Down
1 change: 1 addition & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ extension HTTPClientTests {
("testNoBytesSentOverBodyLimit", testNoBytesSentOverBodyLimit),
("testDoubleError", testDoubleError),
("testSSLHandshakeErrorPropagation", testSSLHandshakeErrorPropagation),
("testWeCloseConnectionsWhenConnectionCloseSetByServer", testWeCloseConnectionsWhenConnectionCloseSetByServer),
]
}
}
26 changes: 26 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2712,4 +2712,30 @@ class HTTPClientTests: XCTestCase {
#endif
}
}

func testWeCloseConnectionsWhenConnectionCloseSetByServer() throws {
let group = DispatchGroup()
group.enter()

let server = try ServerBootstrap(group: self.serverGroup)
.serverChannelOption(ChannelOptions.socketOption(.so_reuseaddr), value: 1)
.childChannelInitializer { channel in
channel.pipeline.configureHTTPServerPipeline().flatMap {
channel.pipeline.addHandler(CloseWithoutClosingServerHandler(group.leave))
}
}
.bind(host: "localhost", port: 0)
.wait()

defer {
server.close(promise: nil)
}

// Simple request, should go great.
XCTAssertNoThrow(try self.defaultClient.get(url: "http://localhost:\(server.localAddress!.port!)/").wait())

// Shouldn't need more than 100ms of waiting to see the close.
let result = group.wait(timeout: DispatchTime.now() + DispatchTimeInterval.milliseconds(100))
XCTAssertEqual(result, .success, "we never closed the connection!")
}
}