Skip to content
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

Remove duplicate Transfer-Encoding header #2229

Closed
wants to merge 1 commit into from

Conversation

rht
Copy link
Contributor

@rht rht commented Jan 21, 2016

Address #2190, #1765.

It is a duplicate since chunked Transfer-Encoding header is already set in http responsewriter.

@rht
Copy link
Contributor Author

rht commented Jan 21, 2016

  • ipfs cat err, which expects chunked transfer encoding even with the content length.

@jbenet
Copy link
Member

jbenet commented Jan 24, 2016

cc @dignifiedquire @whyrusleeping @diasdavid who know most about the encodings stuff

@daviddias
Copy link
Member

Sweet, @RichardLitt had detected this issue during the HTTP-API docs work :)

@rht what do you exactly mean by ipfs cat err, which expects chunked transfer encoding even with the content length.

@rht
Copy link
Contributor Author

rht commented Jan 25, 2016

@diasdavid it's from the travis-ci test: https://travis-ci.org/ipfs/go-ipfs/jobs/103911815#L1458.
I figure that the chunked transfer encoding header doesn't get automatically added due to the request's content-length being explicitly stated.

@rht
Copy link
Contributor Author

rht commented Jan 25, 2016

If this line is removed, then the test is fixed. Should it be removed to make for chunk transfer for every response?

@whyrusleeping
Copy link
Member

@rht we discussed adding an X-Content-Length header for things like cat that like to know the size of the transfer beforehand, but still want to use chunked encoding

@rht
Copy link
Contributor Author

rht commented Jan 30, 2016

"Messages MUST NOT include both a Content-Length header field and a
non-identity transfer-coding. If the message does include a non-
identity transfer-coding, the Content-Length MUST be ignored." [1]

ref:
[1] https://tools.ietf.org/html/rfc2616#section-4.4

@whyrusleeping
Copy link
Member

right, which is why i suggested using X-Content-Length instead. so our code can still get the content length for its progress bar stuff

@rht rht force-pushed the fix/dedup-transfer-encoding-header branch from 73b4985 to 6dbfaa9 Compare February 3, 2016 08:19
And fix content-length conflict as per
https://tools.ietf.org/html/rfc2616#section-4.4

License: MIT
Signed-off-by: rht <rhtbot@gmail.com>
@rht rht force-pushed the fix/dedup-transfer-encoding-header branch from 6dbfaa9 to 785c752 Compare February 3, 2016 08:34
@rht
Copy link
Contributor Author

rht commented Feb 3, 2016

I see what you mean. Fixed. travis-ci test fail is unrelated.

@whyrusleeping
Copy link
Member

fixed in #2465

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants