Skip to content

If not given a content-length, .stream should assume transfer-encoding:chunked #218

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

Closed
weissi opened this issue May 19, 2020 · 6 comments · Fixed by #247
Closed

If not given a content-length, .stream should assume transfer-encoding:chunked #218

weissi opened this issue May 19, 2020 · 6 comments · Fixed by #247
Assignees
Labels
kind/enhancement Improvements to existing feature.
Milestone

Comments

@weissi
Copy link
Contributor

weissi commented May 19, 2020

[THIS SHOULD BE FIXED AFTER #217]

        let request = try! HTTPClient.Request(url: "http://localhost:\(httpBin.port)/get",
                                              method: .POST,
                                              body: .stream { streamWriter in

this Request will actually error (sort of, see #217) but it should just assume transfer-encoding: chunked, the user clearly hasn't given a content length.

If the user manually set a content-length in headers:, then we should obvs use that.

@artemredkin artemredkin added help wanted kind/enhancement Improvements to existing feature. labels May 20, 2020
@artemredkin artemredkin added this to the 1.2.0 milestone May 20, 2020
@artemredkin artemredkin self-assigned this Jun 13, 2020
@artemredkin
Copy link
Collaborator

@weissi do you think we need to prepend length to bytes we get from the client? Or just assume that the client did that?

@weissi
Copy link
Contributor Author

weissi commented Jun 14, 2020

@weissi do you think we need to prepend length to bytes we get from the client? Or just assume that the client did that?

I don’t quite understand the question. What do you want to prepend?

@artemredkin
Copy link
Collaborator

@weissi length of the chunk being written, followed by \r\n

@weissi
Copy link
Contributor Author

weissi commented Jun 15, 2020

@artemredkin that is the HTTP wire protocol. And neither AHC nor the user should think about it. NIO does the HTTP wire protocol (incl chunked encoding) for you. If yoy were to prepend anything in AHC then that would go into the data stream so it would become corrupted.

From AHC’s perspective, you don’t need to worry about chunked or not. You can select it by:

  • add no headers: NIO will assume chunked encoding
  • explicitly add transfer-encoding: chunked, NIO ofc will do chunked
  • add a content-length header in which case NIO will not do chunked ofc

@weissi
Copy link
Contributor Author

weissi commented Jun 15, 2020

@artemredkin a few things that AHC does need to do are:

  • make sure there’s no content-length and transfer-encoding: chunked headers at the same time, that’s illegal
  • if content-length is selected, AHC must make sure that the user is actually supplying exactly that many bytes and no more/less

@artemredkin
Copy link
Collaborator

Ok, got it, thanks! I’ll address body checking when I get to #251

weissi pushed a commit that referenced this issue Jun 23, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants