Skip to content

Commit

Permalink
Address flakiness of testSSLHandshakeErrorPropagation
Browse files Browse the repository at this point in the history
Motivation:

Flaky tests are bad.

This test is flaky because the server closes the connection immediately
upon channelActive. In practice this can mean that the handshake never
even gets a chance to start: by the time the SSLHandler ends up
in the pipeline the connection is already dead. Heck, by the time we
attempt to complete the connection the connection might be dead.

Modifications:

- Change the shutdown to be on first read.
- Remove the disabled autoRead.
- Change the expected NIOTS failure mode to connectTimeout,
    which is how this manifests in NIOTS.

Result:

Test is no longer flaky.
  • Loading branch information
Lukasa committed Jan 25, 2021
1 parent 9671de7 commit f121ddf
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 5 deletions.
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),
("testSSLHandshakeErrorPropagationDelayedClose", testSSLHandshakeErrorPropagationDelayedClose),
("testWeCloseConnectionsWhenConnectionCloseSetByServer", testWeCloseConnectionsWhenConnectionCloseSetByServer),
]
}
Expand Down
65 changes: 60 additions & 5 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2680,13 +2680,12 @@ class HTTPClientTests: XCTestCase {
class CloseHandler: ChannelInboundHandler {
typealias InboundIn = Any

func channelActive(context: ChannelHandlerContext) {
func channelRead(context: ChannelHandlerContext, data: NIOAny) {
context.close(promise: nil)
}
}

let server = try ServerBootstrap(group: self.serverGroup)
.childChannelOption(ChannelOptions.autoRead, value: false)
.childChannelInitializer { channel in
channel.pipeline.addHandler(CloseHandler())
}
Expand All @@ -2697,15 +2696,71 @@ class HTTPClientTests: XCTestCase {
XCTAssertNoThrow(try server.close().wait())
}

let request = try Request(url: "https://localhost:\(server.localAddress!.port!)", method: .GET)
let task = self.defaultClient.execute(request: request, delegate: TestHTTPDelegate())
// We set the connect timeout down very low here because on NIOTS this manifests as a connect
// timeout.
let config = HTTPClient.Configuration(timeout: HTTPClient.Configuration.Timeout(connect: .milliseconds(100), read: nil))
let client = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: config)
defer {
XCTAssertNoThrow(try client.syncShutdown())
}

let request = try Request(url: "https://127.0.0.1:\(server.localAddress!.port!)", method: .GET)
let task = client.execute(request: request, delegate: TestHTTPDelegate())

XCTAssertThrowsError(try task.wait()) { error in
#if os(Linux)
XCTAssertEqual(error as? NIOSSLError, NIOSSLError.uncleanShutdown)
#else
if isTestingNIOTS() {
XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(100)))
} else {
XCTAssertEqual((error as? IOError).map { $0.errnoCode }, 54)
}
#endif
}
}

func testSSLHandshakeErrorPropagationDelayedClose() throws {
// This is as the test above, but the close handler delays its close action by a few hundred ms.
// This will tend to catch the pipeline at different weird stages, and flush out different bugs.
class CloseHandler: ChannelInboundHandler {
typealias InboundIn = Any

func channelRead(context: ChannelHandlerContext, data: NIOAny) {
context.eventLoop.scheduleTask(in: .milliseconds(100)) {
context.close(promise: nil)
}
}
}

let server = try ServerBootstrap(group: self.serverGroup)
.childChannelInitializer { channel in
channel.pipeline.addHandler(CloseHandler())
}
.bind(host: "127.0.0.1", port: 0)
.wait()

defer {
XCTAssertNoThrow(try server.close().wait())
}

// We set the connect timeout down very low here because on NIOTS this manifests as a connect
// timeout.
let config = HTTPClient.Configuration(timeout: HTTPClient.Configuration.Timeout(connect: .milliseconds(200), read: nil))
let client = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: config)
defer {
XCTAssertNoThrow(try client.syncShutdown())
}

let request = try Request(url: "https://127.0.0.1:\(server.localAddress!.port!)", method: .GET)
let task = client.execute(request: request, delegate: TestHTTPDelegate())

XCTAssertThrowsError(try task.wait()) { error in
#if os(Linux)
XCTAssertEqual(error as? NIOSSLError, NIOSSLError.uncleanShutdown)
#else
if isTestingNIOTS() {
XCTAssertEqual((error as? AsyncHTTPClient.HTTPClient.NWTLSError).map { $0.status }, errSSLClosedNoNotify)
XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(200)))
} else {
XCTAssertEqual((error as? IOError).map { $0.errnoCode }, 54)
}
Expand Down

0 comments on commit f121ddf

Please sign in to comment.