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

Consider a more convenient API for responses to HEAD #42

Open
vfaronov opened this issue Mar 25, 2017 · 2 comments
Open

Consider a more convenient API for responses to HEAD #42

vfaronov opened this issue Mar 25, 2017 · 2 comments

Comments

@vfaronov
Copy link

Right now h11 raises a LocalProtocolError if you try to send a non-empty Data event in response to a HEAD request. This can be a nuisance in a catch-all handler, such as the maybe_send_error_response in your Curio example. Notice how there’s no way for maybe_send_error_response to tell that it’s not supposed to send data, because it has no access to the request event, and wrapper.conn.our_state is still SEND_BODY. Handling this cleanly would require some refactoring of the example code. Not to mention that you have to remember HEAD exists in the first place.

Would it perhaps be better if the state machine went from SEND_RESPONSE directly to DONE in this case, so the handler could readily pick that up?

Alternatively, what if h11 just silently ignored the Data events when responding to a HEAD request? (I can see the potential problems with this though: an application could keep “streaming” a long response into the bit bucket; also it’s not clear what to do with trailers if those pop up eventually.)

These are just random ideas not grounded in any serious use of h11; feel free to ignore if this doesn’t sound good.

@vfaronov
Copy link
Author

Also, I just noticed that if an error happens relatively early in the cycle (I guess before the client passes from the IDLE state?), h11 doesn’t enforce the HEAD check and instead happily sends the body. So, if I send this to the Curio example server:

HEAD / HTTP/1.1

I get this:

HTTP/1.1 400 
date: Mon, 27 Mar 2017 19:51:49 GMT
server: h11-example-curio-server/0.7.0+dev python-h11/0.7.0+dev
content-type: text/plain; charset=utf-8
content-length: 30

Missing mandatory Host: header

Not sure if this is a problem or not. It also happens on entirely valid requests, e.g. those where the request-target is longer than the h11 limit.

@njsmith
Copy link
Member

njsmith commented Mar 31, 2017

This is an excellent point that I indeed didn't think about at all :-). Good catch.

I think I'm not worried about the Host: thing, because that client failed to speak HTTP, so who's to say that they even sent a HEAD request?

The case when the headers exceed the maximum size is a bit trickier, but I think there's a limit to how much we should worry about these things... from a quick check, it looks like common production servers like Apache and Google respond to requests like HEAD /<hundreds of kilobytes of path> HTTP/1.0 by sending a 4xx response with an included body (i.e., they don't detect that it's a HEAD request). They do do a bit better than us in that they parse the request line and the headers separately so if the over-large string is in the headers then they leave out the response body, but rewriting our parser just so we can distinguish between bad request lines versus bad header blocks seems like overkill?

I don't much like the idea of silently discarding Data when responding to a HEAD request... that may well make sense for layers on top of h11, like web application servers, but it feels too magic for h11 itself, and error prone due to the issues you mention. Unfortunately for better or worse h11 users do still need to be pretty familiar with HTTP protocol details.

I'm not seeing an immediate obvious change to make here, but I'll think about it.

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

No branches or pull requests

2 participants