Read body for all requests with a Content-Length header

[jgraham]

The framing of a HTTP request doesn't depend on the method [1], so any request with a non-zero Content-Length has a corresponding body. This does not change in the case of idempotent methods like GET; what is different is that for such methods it is incorrect for the server's response to depend on the content of this body.

At the moment GET and HEAD are incorrectly special cased in hyper [2] leading to problems if you send a GET request with a body (the unread body bleeds into the following request unless the connection is closed). In these cases the body data, if any, must be read from the network but could be discarded and not made avaliable through the request API.

[1] https://tools.ietf.org/html/rfc7230#section-3.3
[2] https://github.com/hyperium/hyper/blob/master/src/server/request.rs#L42

[seanmonstar]

Good catch. I've also noticed that for requests that should have a body, such as POST, and the user doesn't read it out, it bleeds into the next request also.

[reem]

We could write a Drop impl for Request that tries to read out the remainder of the body.

[seanmonstar]

cc #197

[seanmonstar]

The patch was reverted, so not fixed.

[reem]

If we are going to do this, it has to be configurable (the max amount we read before shutting down the connection).

[durka]

We should fix this, or advertise the behavior in blinking red marquee in the documentation. I had no idea where to start when I ended up with order-dependent 404 errors in my application (because one POST form submission had content, and the next one had an invalid HTTP method due to this bug).

What scenarios motivate not wanting to read the entire request out of the network buffer?

[SergioBenitez]

I just ran into this issue. This is a rather major issue as it completely breaks Hyper's abstraction over HTTP. That is, it breaks Hyper. If it's not going to be fixed, at least place a warning in the documentation as @durka notes.

[seanmonstar]

The approach I've taken in the async branch is that if a request claims a body, such as using a Content-Length header, you can read it. If a request claims a body, and you have not read it, then the socket will not be marked keep-alive (as the unread body ruins parsing the next request). The body will not be automatically read, so as to give the option to the user.

[SergioBenitez]

Does simply not marking a socket keep-alive solve the issue? My main grievance is not based on whether Hyper should or should not read any remaining data, it's that Hyper fails to parse the next HTTP request correctly, resulting in bogus structures. I don't believe not marking the socket keep-alive prevents this from happening. If that's the case, then this isn't any better than the status-quo. If it does prevent this from happening, then it seems like this issue can be closed.

Can you explain why this can happen at all? Why not fast-forward the stream until after the previous request's content? That is, if a request has been dropped and a new request is reusing that old request's socket, advance the stream by old_Content-Length - (current_read_position - old_headers_size) bytes.

[seanmonstar]

In HTTP/1.x, keep-alive determines whether an additional request/response can occur on the same socket. So, if hyper decides that a connection should not be kept alive, then it will close the socket after the last response is written. A client would then have to arrange a new connection to send another message.

---

The stream isn't necessarily read yet. hyper doesn't perform tcp reads until the user asks for it, when calling request.read(). So when a request is dropped, hyper can check to see if the amount of bytes that have been read from the socket match the Content-Length header. If it does not, then there are still bytes on the wire. Reading until the end would require more socket reads. And what if the Content-Length is something ridiculous, like 2GB. If the user didn't want to read that data, then it's a complete waste of time and bandwidth for hyper to read 2 gigs of bytes to be dropped into the ether.

So then you're left with wondering what is the correct limit that hyper should use to decide if it can "fast forward" or not? Likely, that would be up to the user. Now we have yet another knob to fiddle with. Better to stay explicit: if the user doesn't read the data, hyper doesn't read.

[matt2xu]

For what it's worth, I've been bitten by this bug too: https://github.com/matt2xu/edge-rs/blob/master/src/lib.rs#L175 😄

I agree that your approach is sensible, and all that's missing is documenting this behavior. Frameworks on top of Hyper (or code using Hyper directly) can then decide whether closing the connection or reading the request's body. Ideally I would suggest that in the description of the Request a line be added "It is the user's responsibility to handle the request's body by either reading it or discarding it and closing the connection. Note that by default, Hyper uses a pool of connections with keep-alive behavior, which means that if a request has a body that is not read, the socket is reused and the next request is parsed incorrectly.". What do you think?

[seanmonstar]

It is the user's responsibility to handle the request's body by either reading it or discarding it and closing the connection. Note that by default, Hyper uses a pool of connections with keep-alive behavior, which means that if a request has a body that is not read, the socket is reused and the next request is parsed incorrectly.

While that's true in the current blocking IO code, but it is not that way in the async branch. If a request body is not completely read (in async branch), hyper will drop the socket, not try to parse a new one on it.

[matt2xu]

Ok, good. So we change the text a bit 😄 I still think that this deserves a few words in the documentation so people aren't surprised by this. I look forward to using the async branch by the way, when I have the time I'll update my code.

[durka]

I'm thinking maybe I should publish my Drain adapter as a mini-crate! It's an Iron middleware that reads out an unread request so that the socket can be reused, up to some limit, and if the limit is reached and there's still more data then it closes the socket.

[SergioBenitez]

@seanmonstar

While that's true in the current blocking IO code, but it is not that way in the async branch. If a request body is not completely read (in async branch), hyper will drop the socket, not try to parse a new one on it.

Excellent. That solves the issue for me.

[reem]

Just came across this PR as an example of things that can go wrong from closing the socket if the body is not completely read - google/go-github#317. Not necessarily saying we shouldn't go that way but it's something to keep in mind.

[reem]

@durka definitely release that as its own crate :)

[durka]

@reem done

[seanmonstar]

This has been solved in master.

[untitaker]

@seanmonstar Could you specify the version or commit in which this has been fixed?

[seanmonstar]

@untitaker this was part of the "async" commit, d35992d

The specific logic is here:

hyper/src/http/conn.rs

Lines 623 to 629 in d35992d

	Reading::Body(ref decoder) if decoder.is_eof() => {
	if http1.keep_alive {
	Reading::KeepAlive
	} else {
	Reading::Closed
	}
	},

[golddranks]

Has this fix included in any release yet? I'm encountering the bug with 0.9.10.

[seanmonstar]

Fixing this in 0.9 wasn't wasy to figure out, thanks to the internal design. The rewrite that is included in (not release) 0.10 made fixing this possible.

[golddranks]

Allright, thanks for the info :)

[sapsan4eg]

Guys, issue was open in Feb 2015, almost two years. In hyper 0.10 the problem does not repeat itself, but when the scheduled release? As I understand 0.9x is not compatible with 0.10. And this does not solve the current problem with 0.9x.

[matthauck]

ping. Has this change been released yet?