assume chunked on a stream with no length #247

artemredkin · 2020-06-13T19:23:12Z

Assume chunked encoding if body stream length is nil.

Motivation:
Streams length parameter is optional to allow cases were stream length is not known in advance, but we do not support this in request validation. This PR aims to address that.

Modifications:
Modifies request validation to default to chunked encoding if body length is zero or to passed in content-length header
Adds a test

Result:
Closes #218

weissi · 2020-06-14T18:02:54Z

@artemredkin i’m not quite sure about the description/title of this PR. It shouldn’t do anything special for 0 lenghts. But yes, chunked encoding should always be used unless the user tells us a content length. If the user says length 0 though we should insert a content-length:0 header

artemredkin · 2020-06-14T18:28:15Z

Ah, its totally not about zero length, yes, only if length is not specified, fixed!

weissi · 2020-06-15T10:14:53Z

Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift

@@ -109,6 +109,7 @@ extension HTTPClientTests {
            ("testNothingIsLoggedAtInfoOrHigher", testNothingIsLoggedAtInfoOrHigher),
            ("testAllMethodsLog", testAllMethodsLog),
            ("testClosingIdleConnectionsInPoolLogsInTheBackground", testClosingIdleConnectionsInPoolLogsInTheBackground),
+            ("testUploadStreamingNoLength", testUploadStreamingNoLength),


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.

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

I was planning on doing it in this issue

Amazing, thank you so much!!

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?

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

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

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

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

@weissi done!

* Added additional tests for socketPath-based requests Motivation: While going through the existing tests, I identified a few more instances where we could add some testing. Modifications: Added one test that verifies Requests are being decoded correctly, and improved three others to check for path parsing, error throwing, and schema casing respectively. Result: Tests that continue to pass, but that will also catch any incompatible changes in the future. * Added some convenience initializers to URL and methods to Request for making requests to socket paths Motivation: Creating URLs for connecting to servers bound to socket paths currently requires some additional code to get exactly right. It would be nice to have convenience methods on both URL and Request to assist here. Modifications: - Refactored the get/post/patch/put/delete methods so they all call into a one line execute() method. - Added variations on the above methods so they can be called with socket paths (both over HTTP and HTTPS). - Added public convenience initializers to URL to support the above, and so socket path URLs can be easily created in other situations. - Added unit tests for creating socket path URLs, and testing the new suite of convenience execute methods (that, er, test `HTTPMETHOD`s). (patch, put, and delete are now also tested as a result of these tests) - Updated the read me with basic usage instructions. Result: New methods that allow for easily creating requests to socket paths, and passing tests to go with them. * Removed some of the new public methods added for creating a socket-path based request Motivation: I previously added too much new public API that will most likely not be necessary, and can be better accessed using a generic execute method. Modifications: Removed the get/post/patch/put/delete methods that were specific to socket paths. Result: Less new public API. * Renamed execute(url:) methods such that the HTTP method is the first argument in the parameter list Motivation: If these are intended to be general methods for building simple requests, then it makes sense to have the method be the first parameter in the list. Modifications: Moved the `method: HTTPMethod` parameter to the front of the list for all `execute([...] url: [...])` methods, and made it default to .GET. I also changed the url parameter to be `urlPath` for the two socketPath based execute methods. Result: A cleaner public interface for users of the API. * Fixed some minor issues introduces with logging Motivation: Some of the convenience request methods weren't properly adapted for logging. Modifications: - Removed a doc comment from patch() that incorrectly referenced a logger. - Fixed an issue where patch() would call into post(). - Added a doc comment to delete() that references the logger. - Tests for the above come in the next commit... Result: Correct documentation and functionality for the patch() and delete() methods. * Updated logging tests to also check the new execute methods Motivation: The logging tests previously didn't check for socket path-based requests. Modifications: Updated the `testAllMethodsLog()` and `testAllMethodsLog()` tests to include checks for each of the new `execute()` methods. Result: Two more tests that pass.

…wift-server#268)

* fix missing connect timeout and make tests safer * swiftformat and linux tests * fix timeout test * speedup another test * make tests safer

`redirectConfiguration` can't default to `false` as it's not a boolean value, and the default value is `RedirectConfiguration()`. Co-authored-by: Johannes Weiss <[email protected]> Co-authored-by: Artem Redkin <[email protected]>

…server was always allowed (swift-server#259) * Added tests that ensure redirects to unix socket paths from a regular HTTP server are disallowed. Motivation: Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See swift-server#230. Modifications: - Added a method to HTTPBin to redirect to the specified target. - Added failing tests that perform redirects from a regular server to a socket-based server and vice versa. Result: Failing tests that show that the existing redirect checks were inadequate. * Fixed an issue where redirects to socket path-based servers from any server was always allowed. Motivation: An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme. Modifications: - Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do. - Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now. - Check the destination URL's scheme rather than the current URL's scheme when validating a redirect. Result: Closes swift-server#230 Co-authored-by: Artem Redkin <[email protected]>

weissi

Wow, this is amazing. Thank you!!

assume chunked on zero-length stream

2383b65

artemredkin requested a review from weissi June 13, 2020 19:23

linux test and swift format

084dee8

artemredkin changed the title ~~assume chunked on zero-length stream~~ assume chunked on a stream with no length Jun 14, 2020

Lukasa approved these changes Jun 15, 2020

View reviewed changes

artemredkin added this to the 1.2.0 milestone Jun 15, 2020

Merge branch 'master' into assume_chunked_on_zero_length_stream

d8fad2d

weissi reviewed Jun 15, 2020

View reviewed changes

weissi and others added 12 commits June 17, 2020 17:05

Merge branch 'master' into assume_chunked_on_zero_length_stream

83a83e9

wip

9a431d1

fix flaky testContentLengthTooLongFails test (swift-server#269)

eaf203e

All internal connection flow should be executed when shutting down (s…

02b3ed9

…wift-server#268)

fix missing connect timeout and make tests safer (swift-server#267)

9f65902

* fix missing connect timeout and make tests safer * swiftformat and linux tests * fix timeout test * speedup another test * make tests safer

notify delegate about connect errors (swift-server#245)

29dc2c7

make all tests pass

ac8b433

Merge branch 'master' into assume_chunked_on_zero_length_stream

aad8fa6

swiftformat

08c7a33

artemredkin requested a review from weissi June 23, 2020 14:30

weissi approved these changes Jun 23, 2020

View reviewed changes

weissi merged commit 61a80a2 into swift-server:master Jun 23, 2020

artemredkin deleted the assume_chunked_on_zero_length_stream branch June 23, 2020 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assume chunked on a stream with no length #247

assume chunked on a stream with no length #247

artemredkin commented Jun 13, 2020 •

edited

Loading

weissi commented Jun 14, 2020

artemredkin commented Jun 14, 2020

weissi Jun 15, 2020

weissi Jun 17, 2020

artemredkin Jun 17, 2020

weissi Jun 17, 2020

artemredkin Jun 19, 2020

artemredkin Jun 19, 2020

artemredkin Jun 19, 2020

weissi Jun 22, 2020

weissi Jun 22, 2020

artemredkin Jun 23, 2020

weissi left a comment

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

assume chunked on a stream with no length #247

assume chunked on a stream with no length #247

Conversation

artemredkin commented Jun 13, 2020 • edited Loading

weissi commented Jun 14, 2020

artemredkin commented Jun 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

artemredkin commented Jun 13, 2020 •

edited

Loading