Read GET/HEAD body if it exists

[oschwald]

Previously, GET/HEAD bodies were not read. The HTTP 1.1 specification
states:

"A server SHOULD read and forward a message-body on any request; if the
request method does not include defined semantics for an entity-body,
then the message-body SHOULD be ignored when handling the request.
I suspect this code is at fault."

This change reads the body on such request and continues the previous
behavior of returning a "Content-Length" of 0 to the application.

Closes #159.

[oschwald]

I would be happy to modify this to completely discard the body as well if that is the preferred behavior. That might be more in line with the spec. Just let me know.

[valyala]

What is the use case for this feature?
Which clients allow sending bodies in GET or HEAD requests?

[oschwald]

Unfortunately, there are quite a few real world GET requests that have a
body. Many clients allow this. Currently, fasthttp incorrectly drops the
connection and logs an error. This PR brings fasthttp in line with net/http
and the HTTP/1.1 spec. As I mentioned, I would be happy to modify the PR to
discard the body rather than putting it in the buffer.

On Wed, Oct 5, 2016, 3:23 AM Aliaksandr Valialkin [email protected]
wrote:

What is the use case for this feature?
Which clients allow sending bodies in GET or HEAD requests?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#171 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARBM75WGJcrulK-Y1REgjCOuv-FE50Oks5qw3q1gaJpZM4J9Uhu
.

[ghost]

You can send a body with GET, but it's never useful to do so.

This is part of the layered design of HTTP/1.1 will become clear again once the spec is partitioned, future changes may discard this option

[oschwald]

I agree it is not useful to send a body. The intent of this PR is not to
make the body useful. It is to stop fasthttp from dropping the connection
prematurely and logging an error.

On Wed, Oct 5, 2016, 4:42 AM Gerasimos Maropoulos [email protected]
wrote:

You can send a body with GET, but it's never useful to do so.

This is part of the layered design of HTTP/1.1 will become clear again
once the spec is partitioned, future changes may discard this option

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#171 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARBMzIZ7prfO2Md7BYrGvI6oViwPp-7ks5qw40tgaJpZM4J9Uhu
.

[oschwald]

An alternative to this PR would be to return a 4xx error on GET requests with a body. net/http might switch to that behavior in the future, golang/go#13722. This would be more inconvenient for me personally (as we have clients sending us this stuff), but it would be an improvement over the current behavior where the connection hangs until it times out.

[docmerlin]

We are also having this same problem. We are losing connections due to this bug.

[docmerlin]

'What is the use case for this feature?
Which clients allow sending bodies in GET or HEAD requests?'

• several.

Furthermore, it violates the robustness principle to not accept such requests.
"be liberal in what you accept, and conservative in what you send"

[docmerlin]

Almost all our requests to this service come from JS on browsers, and we are missing quite a few due to this bug. Please accept a patch.

[theiostream]

Please accept this patch.

[hails]

Please accept this patch.

[filipedeschamps]

I also need this feature!

[greenboxal]

+1
Although it's not in official spec, this makes sense nowadays to some payloads. A good example is an Elasticsearch query which makes more sense as a GET as I'm searching for something but it has a really long payload that doesn't fit well in the query string.

[erikdubbelboer]

I merged the pull request into my branch which contains other important fixes such as erikdubbelboer@28a6163 as well.

[oschwald]

Thanks, @erikdubbelboer! I've switched to using your fork.

[tsingson]

thanks for the patch .

[erikdubbelboer]

Was fixed in cf6f6e7

[oschwald]

Thanks! I'll close this.