Skip to content

assume chunked on a stream with no length #247

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

assume chunked on a stream with no length #247

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2383b65
assume chunked on zero-length stream
artemredkin Jun 13, 2020
084dee8
linux test and swift format
artemredkin Jun 13, 2020
d8fad2d
Merge branch 'master' into assume_chunked_on_zero_length_stream
artemredkin Jun 15, 2020
83a83e9
Merge branch 'master' into assume_chunked_on_zero_length_stream
weissi Jun 17, 2020
995a98b
Convenience methods for socket paths (#235)
dimitribouniol Jun 19, 2020
9a431d1
wip
artemredkin Jun 19, 2020
eaf203e
fix flaky testContentLengthTooLongFails test (#269)
artemredkin Jun 22, 2020
02b3ed9
All internal connection flow should be executed when shutting down (#…
artemredkin Jun 22, 2020
9f65902
fix missing connect timeout and make tests safer (#267)
artemredkin Jun 22, 2020
f6f99d2
Fix doc comment for redirectConfiguration (#266)
MaxDesiatov Jun 22, 2020
233f18b
Fixed an issue where redirects to socket path-based servers from any …
dimitribouniol Jun 22, 2020
29dc2c7
notify delegate about connect errors (#245)
artemredkin Jun 23, 2020
ac8b433
make all tests pass
artemredkin Jun 23, 2020
aad8fa6
Merge branch 'master' into assume_chunked_on_zero_length_stream
artemredkin Jun 23, 2020
08c7a33
swiftformat
artemredkin Jun 23, 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
12 changes: 8 additions & 4 deletions Sources/AsyncHTTPClient/RequestValidation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ extension HTTPHeaders {
}

self.remove(name: "Transfer-Encoding")
self.remove(name: "Content-Length")

try self.validateFieldNames()

guard let body = body else {
self.remove(name: "Content-Length")
// if we don't have a body we might not need to send the Content-Length field
// https://tools.ietf.org/html/rfc7230#section-3.3.2
switch method {
Expand Down Expand Up @@ -60,11 +60,15 @@ extension HTTPHeaders {
}

if encodings.isEmpty {
guard let length = body.length else {
throw HTTPClientError.contentLengthMissing
if let length = body.length {
self.remove(name: "Content-Length")
contentLength = length
} else if !self.contains(name: "Content-Length") {
transferEncoding = "chunked"
}
contentLength = length
} else {
self.remove(name: "Content-Length")

transferEncoding = encodings.joined(separator: ", ")
if !encodings.contains("chunked") {
guard let length = body.length else {
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 @@ -109,6 +109,7 @@ extension HTTPClientTests {
("testNothingIsLoggedAtInfoOrHigher", testNothingIsLoggedAtInfoOrHigher),
("testAllMethodsLog", testAllMethodsLog),
("testClosingIdleConnectionsInPoolLogsInTheBackground", testClosingIdleConnectionsInPoolLogsInTheBackground),
("testUploadStreamingNoLength", testUploadStreamingNoLength),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a few more test cases here. Essentially we have quite a big matrix of possibilities:

HTTP Method kind User sets Body Expectation
.GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing nil Neither CL nor chunked
other nothing nil chunked
.GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length nil CL=0
other content-length nil CL=0
.GET, .HEAD, .DELETE, .CONNECT, .TRACE transfer-encoding: chunked nil chunked
other transfer-encoding: chunked nil chunked
.GET, .HEAD, .DELETE, .CONNECT, .TRACE CL & chunked (illegal) nil throws error
other CL & chunked (illegal) nil throws error
.GET, .HEAD, .DELETE, .CONNECT, .TRACE nothing not nil Neither CL nor chunked
other nothing not nil chunked
.GET, .HEAD, .DELETE, .CONNECT, .TRACE content-length not nil CL
other content-length not nil CL
.GET, .HEAD, .DELETE, .CONNECT, .TRACE transfer-encoding: chunked not nil chunked
other transfer-encoding: chunked not nil chunked
.GET, .HEAD, .DELETE, .CONNECT, .TRACE CL & chunked (illegal) not nil throws error
other CL & chunked (illegal) not nil throws error

I think we need to test them systematically or else we'll get more bugs here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@artemredkin should we do this in this ticket or create an issue from it? I think it's important because the code looks tricky

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was planning on doing it in this issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing, thank you so much!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, I'm not sure some combinations are correct, for example:

Method Header Body Expectation
other nothing nil chunked

but there is this bit in https://tools.ietf.org/html/rfc7230#section-3.3.2:

   no Transfer-Encoding is sent and the request method defines a meaning
   for an enclosed payload body.  For example, a Content-Length header
   field is normally sent in a POST request even when the value is 0
   (indicating an empty payload body).

shouldn't this be CL=0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And, if users sets chunked but with nil body, we know there will be zero bytes, so we should default to Neither CL nor chunked for .GET and to CL=0 for other

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And last point is when user sets nothing but provides a body, we again can do two things:
if body length known - use CL, otherwise - chunked

Copy link
Contributor

Choose a reason for hiding this comment

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

@artemredkin sorry, I made a number of C&P errors when building this table :). I agree with all the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should only expect chunked if the user has told us that they have a body but they didn't tell us the length.

We should set CL=0 if the user set body: nil UNLESS it's .GET, .HEAD, .DELETE, .CONNECT, .TRACE

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 done!

]
}
}
31 changes: 31 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2005,4 +2005,35 @@ class HTTPClientTests: XCTestCase {

self.defaultClient = nil // so it doesn't get shut down again.
}

func testUploadStreamingNoLength() throws {
let server = NIOHTTP1TestServer(group: self.serverGroup)
let client = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup))
defer {
XCTAssertNoThrow(try client.syncShutdown())
XCTAssertNoThrow(try server.stop())
}

var request = try HTTPClient.Request(url: "http://localhost:\(server.serverPort)/")
request.body = .stream { writer in
writer.write(.byteBuffer(ByteBuffer.of(string: "1234")))
}

let future = client.execute(request: request)

switch try server.readInbound() {
case .head(let head):
XCTAssertEqual(head.headers["transfer-encoding"], ["chunked"])
default:
XCTFail("Unexpected part")
}

XCTAssertNoThrow(try server.readInbound()) // .body
XCTAssertNoThrow(try server.readInbound()) // .end

XCTAssertNoThrow(try server.writeOutbound(.head(.init(version: .init(major: 1, minor: 1), status: .ok))))
XCTAssertNoThrow(try server.writeOutbound(.end(nil)))

XCTAssertNoThrow(try future.wait())
}
}