Skip to content

fix missing connect timeout and make tests safer #267

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.

Sign up for GitHub

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 7 commits into from
Jun 22, 2020
Merged

fix missing connect timeout and make tests safer #267

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d5b61f2
fix missing connect timeout and make tests safer
artemredkin Jun 19, 2020
f099d6f
swiftformat and linux tests
artemredkin Jun 19, 2020
ee1d5a8
fix timeout test
artemredkin Jun 19, 2020
8eeae12
speedup another test
artemredkin Jun 19, 2020
5a43892
make tests safer
artemredkin Jun 22, 2020
96abd0e
Merge branch 'master' into fix_missing_connect_timeout
artemredkin Jun 22, 2020
1e58192
Merge branch 'master' into fix_missing_connect_timeout
artemredkin Jun 22, 2020
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
7 changes: 6 additions & 1 deletion Sources/AsyncHTTPClient/Utils.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ extension NIOClientTCPBootstrap {
requiresTLS: Bool,
configuration: HTTPClient.Configuration
) throws -> NIOClientTCPBootstrap {
let bootstrap: NIOClientTCPBootstrap
var bootstrap: NIOClientTCPBootstrap
#if canImport(Network)
// if eventLoop is compatible with NIOTransportServices create a NIOTSConnectionBootstrap
if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), let tsBootstrap = NIOTSConnectionBootstrap(validatingGroup: eventLoop) {
Expand All @@ -106,10 +106,15 @@ extension NIOClientTCPBootstrap {
}
#endif

if let timeout = configuration.timeout.connect {
bootstrap = bootstrap.connectTimeout(timeout)
}

// don't enable TLS if we have a proxy, this will be enabled later on
if requiresTLS, configuration.proxy == nil {
return bootstrap.enableTLS()
}

return bootstrap
}

Expand Down
28 changes: 14 additions & 14 deletions Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,24 @@ class HTTPClientNIOTSTests: XCTestCase {
func testConnectionFailError() {
guard isTestingNIOTS() else { return }
let httpBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup))
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup),
configuration: .init(timeout: .init(connect: .milliseconds(100),
read: .milliseconds(100))))

defer {
XCTAssertNoThrow(try httpClient.syncShutdown(requiresCleanClose: true))
}

let port = httpBin.port
XCTAssertNoThrow(try httpBin.shutdown())

do {
_ = try httpClient.get(url: "https://localhost:\(port)/get").wait()
XCTFail("This should have failed")
} catch ChannelError.connectTimeout {
} catch {
XCTFail("Error should have been ChannelError.connectTimeout not \(type(of: error))")
XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) { error in
switch error {
case ChannelError.connectTimeout(let timeout):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weissi do you know if there is a safer pattern for this case, where enum has an associated value?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What don't you like about this pattern?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially dangerous, if you miss the XCTFail call in the default clause, you might miss when the test breaks. This is why I'm replacing do-try-XCTFail-catch pattern with XCTAssertThrowsError

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. No, there isn't really a better pattern here sadly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually do this

XCTAssertEqul(.connectTimeout(.milliSeconds(150)), error as? ChannelError)

as it's much safer

XCTAssertLessThanOrEqual(timeout, .milliseconds(150))
default:
XCTFail("Unexpected error: \(error)")
}
}
}

Expand All @@ -103,13 +108,8 @@ class HTTPClientNIOTSTests: XCTestCase {
XCTAssertNoThrow(try httpBin.shutdown())
}

do {
_ = try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait()
XCTFail("This should have failed")
} catch let error as HTTPClient.NWTLSError {
XCTAssertEqual(error.status, errSSLHandshakeFail)
} catch {
XCTFail("Error should have been NWTLSError not \(type(of: error))")
XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait()) { error in
XCTAssertEqual((error as? HTTPClient.NWTLSError)?.status, errSSLHandshakeFail)
}
#endif
}
Expand Down
1 change: 1 addition & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ extension HTTPClientTests {
("testStreaming", testStreaming),
("testRemoteClose", testRemoteClose),
("testReadTimeout", testReadTimeout),
("testConnectTimeout", testConnectTimeout),
("testDeadline", testDeadline),
("testCancel", testCancel),
("testStressCancel", testStressCancel),
Expand Down
22 changes: 22 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,28 @@ class HTTPClientTests: XCTestCase {
}
}

func testConnectTimeout() throws {
let httpBin = HTTPBin(ssl: false)
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup),
configuration: .init(timeout: .init(connect: .milliseconds(100), read: .milliseconds(150))))

defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
}

let port = httpBin.port
XCTAssertNoThrow(try httpBin.shutdown())

XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) { error in
switch error {
case ChannelError.connectTimeout(let timeout):
XCTAssertLessThanOrEqual(timeout, .milliseconds(150))
default:
XCTFail("Unexpected error: \(error)")
}
}
}

func testDeadline() throws {
XCTAssertThrowsError(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "wait", deadline: .now() + .milliseconds(150)).wait(), "Should fail") { error in
guard case let error = error as? HTTPClientError, error == .readTimeout else {
Expand Down