Skip to content

Fix potentially flaky HTTPClientTests.testNoBytesSentOverBodyLimit #426

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
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
59 changes: 41 additions & 18 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -968,11 +968,8 @@ class HTTPClientTests: XCTestCase {
request.body = .byteBuffer(ByteBuffer(bytes: [120, 156, 75, 76, 28, 5, 200, 0, 0, 248, 66, 103, 17]))
request.headers.add(name: "Accept-Encoding", value: "deflate")

XCTAssertThrowsError(try localClient.execute(request: request).wait()) { error in
guard case .some(.limit) = error as? NIOHTTPDecompression.DecompressionError else {
XCTFail("wrong error: \(error)")
return
}
XCTAssertThrowsError(try localClient.execute(request: request).wait()) {
XCTAssertEqual($0 as? NIOHTTPDecompression.DecompressionError, .limit)
}
}

Expand Down Expand Up @@ -2644,19 +2641,45 @@ class HTTPClientTests: XCTestCase {
}

let tooLong = "XBAD BAD BAD NOT HTTP/1.1\r\n\r\n"
let future = self.defaultClient.execute(
request: try Request(url: "http://localhost:\(server.serverPort)",
body: .stream(length: 1) { streamWriter in
streamWriter.write(.byteBuffer(ByteBuffer(string: tooLong)))
}))

XCTAssertNoThrow(try server.readInbound()) // .head
// this should fail if client detects that we are about to send more bytes than body limit and closes the connection
// We can test that this test actually fails if we remove limit check in `writeBodyPart` - it will send bytes, meaning that the next
// call will not throw, but the future will still throw body mismatch error
XCTAssertThrowsError(try server.readInbound()) { error in XCTAssertEqual(error as? HTTPParserError, HTTPParserError.invalidEOFState) }

XCTAssertThrowsError(try future.wait())

let request = try Request(
url: "http://localhost:\(server.serverPort)",
body: .stream(length: 1) { streamWriter in
streamWriter.write(.byteBuffer(ByteBuffer(string: tooLong)))
}
)

let future = self.defaultClient.execute(request: request)

// Okay, what happens here needs an explanation:
//
// In the request state machine, we should start the request, which will lead to an
// invocation of `context.write(HTTPRequestHead)`. Since we will receive a streamed request
// body a `context.flush()` will be issued. Further the request stream will be started.
// Since the request stream immediately produces to much data, the request will be failed
// and the connection will be closed.
//
// Even though a flush was issued after the request head, there is no guarantee that the
// request head was written to the network. For this reason we must accept not receiving a
// request and receiving a request head.

do {
_ = try server.receiveHead()

// A request head was sent. We expect the request now to fail with a parsing error,
// since the client ended the connection to early (from the server's point of view.)
XCTAssertThrowsError(try server.readInbound()) {
XCTAssertEqual($0 as? HTTPParserError, HTTPParserError.invalidEOFState)
}
} catch {
// TBD: We sadly can't verify the error type, since it is private in `NIOTestUtils`:
// NIOTestUtils.BlockingQueue<Element>.TimeoutError
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably find a way to make this public so we can be confident that we're throwing the right error.

}

// request must always be failed with this error
XCTAssertThrowsError(try future.wait()) {
XCTAssertEqual($0 as? HTTPClientError, .bodyLengthMismatch)
}
}

func testDoubleError() throws {
Expand Down