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

Allows the presence of 'content-length' header for chunked transfers. #115

Merged
merged 2 commits into from Apr 7, 2015

Conversation

Projects
None yet
2 participants
@lostbean
Contributor

lostbean commented Apr 6, 2015

After ed0b0b7 , the presence of both 'content-length' and 'transfer-encoding: chunked' rises an error. Although this is the ideal solution, there some real word, non-RFC 2616 servers out there that send chunkend bodies with 'content-length' headers.

This PR basically reverts ed0b0b7. The goal is to accept a chunked transfer even if the 'content-length' is present by ignoring its presence. In other words, the body acquisition should proceed as in any other chunked transfer.

@lostbean

This comment has been minimized.

Show comment
Hide comment
@lostbean

lostbean Apr 6, 2015

Contributor

I've noticed that the original test case uses an broken chunked string:

        N.appWrite ad "HTTP/1.1 200 OK\r\ncontent-length: 0\r\ntransfer-encoding: chunked\r\n\r\n0"

The last (or unique) chunk of data has to be "0\r\n\r\n" (i.e. two CRLF). The original test makes the body reconstruction fails

*** Exception: recv: resource vanished (Connection reset by peer)

Although an exception is certainly expected in this situation, I would expect 'InvalidChunkHeaders' instead. Am I missing something here?

Contributor

lostbean commented Apr 6, 2015

I've noticed that the original test case uses an broken chunked string:

        N.appWrite ad "HTTP/1.1 200 OK\r\ncontent-length: 0\r\ntransfer-encoding: chunked\r\n\r\n0"

The last (or unique) chunk of data has to be "0\r\n\r\n" (i.e. two CRLF). The original test makes the body reconstruction fails

*** Exception: recv: resource vanished (Connection reset by peer)

Although an exception is certainly expected in this situation, I would expect 'InvalidChunkHeaders' instead. Am I missing something here?

@snoyberg

View changes

Show outdated Hide outdated http-client/Network/HTTP/Client/Types.hs Outdated
@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Apr 7, 2015

Owner

Looks good, I only had one comment. The reason no exception is thrown for the invalid chunking is similar to what you're doing here: I'm trying to be lenient in what we accept.

Owner

snoyberg commented Apr 7, 2015

Looks good, I only had one comment. The reason no exception is thrown for the invalid chunking is similar to what you're doing here: I'm trying to be lenient in what we accept.

@lostbean

This comment has been minimized.

Show comment
Hide comment
@lostbean

lostbean Apr 7, 2015

Contributor

I completely agree with being lenient, but my observation is about something else. Removing "\r\n\r\n" from the last chunk (as in the original test case) rises an error in the current implementation of makeChunkedReader, but IMHO it is the wrong type of error. We should do one of the following:

  1. recognize that this is the last chunk if the ckunk header is "0" or "0/rxxxxx" and then return the body without any error.
  2. throw InvalidChunkHeaders or IncompleteHeaders which would give a clear indication about the problem.

Apparently the function recv (connectionRead in http-client) is the one to blame here. It should return empty instead of rising an error when trying to read after the end of the stream. The documentation of recv says For TCP sockets, a zero length return value means the peer has closed its half side of the connection.

Does it make any sense ? :)

Contributor

lostbean commented Apr 7, 2015

I completely agree with being lenient, but my observation is about something else. Removing "\r\n\r\n" from the last chunk (as in the original test case) rises an error in the current implementation of makeChunkedReader, but IMHO it is the wrong type of error. We should do one of the following:

  1. recognize that this is the last chunk if the ckunk header is "0" or "0/rxxxxx" and then return the body without any error.
  2. throw InvalidChunkHeaders or IncompleteHeaders which would give a clear indication about the problem.

Apparently the function recv (connectionRead in http-client) is the one to blame here. It should return empty instead of rising an error when trying to read after the end of the stream. The documentation of recv says For TCP sockets, a zero length return value means the peer has closed its half side of the connection.

Does it make any sense ? :)

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Apr 7, 2015

Owner

I don't think the current behavior is incorrect, or even misleading. There's no real way to distinguish "the server purposefully sent an invalid chunk and then closed the connection" from "the connection was terminated before the end of the chunk header arrived." I don't see a problem with your approach either, but I'm not inclined to write code to make that change, and given that the change you're proposing would have some possibly far-reaching impacts on the rest of the codebase, I'd be hesitant to merge it in.

Owner

snoyberg commented Apr 7, 2015

I don't think the current behavior is incorrect, or even misleading. There's no real way to distinguish "the server purposefully sent an invalid chunk and then closed the connection" from "the connection was terminated before the end of the chunk header arrived." I don't see a problem with your approach either, but I'm not inclined to write code to make that change, and given that the change you're proposing would have some possibly far-reaching impacts on the rest of the codebase, I'd be hesitant to merge it in.

snoyberg added a commit that referenced this pull request Apr 7, 2015

Merge pull request #115 from lostbean/length-and-chunk
Allows the presence of 'content-length' header for chunked transfers.

@snoyberg snoyberg merged commit 9eea89a into snoyberg:master Apr 7, 2015

@lostbean

This comment has been minimized.

Show comment
Hide comment
@lostbean

lostbean Apr 7, 2015

Contributor

Indeed, both situations are indistinguishable and I understand that my propose might not worth to be implemented. I'm fine with this conclusion. I just wanted to bring this issue to light and have your opinion.

Thank you for merging this PR upstream!

Contributor

lostbean commented Apr 7, 2015

Indeed, both situations are indistinguishable and I understand that my propose might not worth to be implemented. I'm fine with this conclusion. I just wanted to bring this issue to light and have your opinion.

Thank you for merging this PR upstream!

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Apr 7, 2015

Owner

No problem, thank you for the PR!

On Tue, Apr 7, 2015 at 6:40 PM Edgar Gomes notifications@github.com wrote:

Indeed, both situations are indistinguishable and I understand that my
propose might not worth to be implemented. I'm fine with this conclusion. I
just wanted to bring this issue to light and have your opinion.

Thank you for merging this PR upstream!


Reply to this email directly or view it on GitHub
#115 (comment).

Owner

snoyberg commented Apr 7, 2015

No problem, thank you for the PR!

On Tue, Apr 7, 2015 at 6:40 PM Edgar Gomes notifications@github.com wrote:

Indeed, both situations are indistinguishable and I understand that my
propose might not worth to be implemented. I'm fine with this conclusion. I
just wanted to bring this issue to light and have your opinion.

Thank you for merging this PR upstream!


Reply to this email directly or view it on GitHub
#115 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment