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

Headers differ between HEAD and GET requests #354

Closed
DennisMcWherter opened this issue Mar 31, 2015 · 22 comments
Closed

Headers differ between HEAD and GET requests #354

DennisMcWherter opened this issue Mar 31, 2015 · 22 comments

Comments

@DennisMcWherter
Copy link
Contributor

I noticed today that the headers differ between HEAD and GET requests for the same resource. Is there a reason for this? The RFC suggests that these headers be the same unless there exists a strong reason or special circumstance otherwise (i.e. the use of "SHOULD").

Here are the results of a local test:

$ curl -4 -i -X HEAD localhost:3000/wai/LICENSE
HTTP/1.1 200 OK
Date: Tue, 31 Mar 2015 01:07:02 GMT
Server: Warp/3.0.10.1
Content-Type: application/octet-stream
last-modified: Sun, 29 Mar 2015 16:31:40 GMT
$ curl -4 -i -X GET localhost:3000/wai/LICENSE
HTTP/1.1 200 OK
Content-Length: 1085
Accept-Ranges: bytes
Date: Tue, 31 Mar 2015 01:07:39 GMT
Server: Warp/3.0.10.1
Content-Type: application/octet-stream
last-modified: Sun, 29 Mar 2015 16:31:40 GMT

... Truncated content

This feature is particularly useful in conjunction with #345 (i.e. having Content-Length and Content-Type).

@snoyberg
Copy link
Member

What application are you running?

On Tue, Mar 31, 2015, 4:10 AM Dennis McWherter notifications@github.com
wrote:

I noticed today that the headers differ between HEAD and GET requests for
the same resource. Is there a reason for this? The RFC
http://tools.ietf.org/html/rfc2616#section-9.4 suggests that these
headers be the same unless there exists a strong reason or special
circumstance otherwise (i.e. the use of "SHOULD").

Here are the results of a local test:

$ curl -4 -i -X HEAD localhost:3000/wai/LICENSE
HTTP/1.1 200 OK
Date: Tue, 31 Mar 2015 01:07:02 GMT
Server: Warp/3.0.10.1
Content-Type: application/octet-stream
last-modified: Sun, 29 Mar 2015 16:31:40 GMT

$ curl -4 -i -X GET localhost:3000/wai/LICENSE
HTTP/1.1 200 OK
Content-Length: 1085
Accept-Ranges: bytes
Date: Tue, 31 Mar 2015 01:07:39 GMT
Server: Warp/3.0.10.1
Content-Type: application/octet-stream
last-modified: Sun, 29 Mar 2015 16:31:40 GMT

... Truncated content

This feature is particularly useful in conjunction with #345
#345 (i.e. having Content-Length
and Content-Type).


Reply to this email directly or view it on GitHub
#354.

@DennisMcWherter
Copy link
Contributor Author

Good question (made me dig a little deeper into the code). wai-app-static, I believe; I am just running the warp binary directly. That said, after reading the source more closely it looks like this really should be giving a 405 in this application (I have tested that this is the case for POST, PUT, DELETE).

I am just starting to dig into the wai code (so I am largely unfamiliar with it at this point), but I suspect that this issue is possibly coming from a bug in parsing the request data (i.e. requestMethod for a HEAD request is being assigned H.methodGet). I will investigate further when I have a chance a little bit later today.

@snoyberg
Copy link
Member

It looks to me like you're encountering the behavior of the autohead middleware, which automatically translates a HEAD request into a GET request. It looks like that middleware should be made smarter in the case of a file response. This may actually be trivial...

@snoyberg
Copy link
Member

Nope, not trivial. Looks like Warp also needs some tweaking around how it handles no-body responses to handle this correctly. It's not terribly involved, but more time than I have this week. If you're interested in diving into the wai codebase some more, this is a great place to do it actually. Check out Network/Wai/Handler/Warp/Response.hs, and look for methodHead.

@DennisMcWherter
Copy link
Contributor Author

That sounds great (or not since that means something needs to be fixed). I actually found this while looking for some task to get started in the codebase, so I will use this as a chance to dive in :) Thanks for the pointer to source.

@snoyberg
Copy link
Member

Awesome, welcome aboard :) Let me know if you have any questions.

On Tue, Mar 31, 2015 at 4:25 PM Dennis McWherter notifications@github.com
wrote:

That sounds great (or not since that means something needs to be fixed). I
actually found this while looking for some task to get started in the
codebase, so I will use this as a chance to dive in :)


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

@kazu-yamamoto
Copy link
Contributor

I don't know about wai-app-static but Mighty has the same behavior. This is because responseLBS is used for HEAD while ResponseFile is used for GET.

Since warp cannot tell responseLBS is used for HEAD, wai applications themselves should add Content-Length: and so on. I'm not sure this is worth fixing.

@DennisMcWherter
Copy link
Contributor Author

Sorry, I've been busy at work the last day (and may be through tonight as well). In any case, I've been reading the code as much as I can and see what you mean kazu.

That said, I would suggest alternate behavior where the user specifies the file with a HEAD request (i.e. the same way it is done with a GET request) and warp responds the same way except it does not send the content body (i.e. just bind the composed headers with connSendAll like your usual sendResponseNoBody). Relying on the user to ensure they match warp's headers seems painful to me.

Even so, if it's not worth fixing, I'll start looking at other things :)

@kazu-yamamoto
Copy link
Contributor

To implement that behavior, I think that we need to change
ResponseFile Status ResponseHeaders FilePath (Maybe FilePart)
to
ResponseFile Status ResponseHeaders FilePath (Maybe FilePart) Bool
where the last Bool tells whether or not the file is actually sent.

Or we need to prepare another constructor.

@snoyberg
Copy link
Member

snoyberg commented Apr 1, 2015

I don't think we need to change anything. Handlers should be able to notice
that the request method is HEAD and simply not send a body.

On Wed, Apr 1, 2015 at 4:53 PM Kazu Yamamoto notifications@github.com
wrote:

To implement that behavior, I think that we need to change
ResponseFile Status ResponseHeaders FilePath (Maybe FilePart)
to
ResponseFile Status ResponseHeaders FilePath (Maybe FilePart) Bool
where the last Bool tells whether or not the file is actually sent.

Or we need to prepare another constructor.


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

@kazu-yamamoto
Copy link
Contributor

Yup, that is one possibility. We should chose one:

    1. Changing API
      -- Pros: clean semantics
      -- Cons: breaking backward compatibility
    1. Using a special header
      -- Pros: maintaining backward compatibility
      -- Cons: this is a hack

So, let's decide which one is better. If this is really important, I like 1).

@snoyberg
Copy link
Member

snoyberg commented Apr 1, 2015

My point is that I don't think we need any special headers; the client is already setting the request method to HEAD in this case, we just need Warp to have respect for it (which it already has, it just needs to be tweaked).

@kazu-yamamoto
Copy link
Contributor

I read "Handlers" as "Headers". Sorry.

Yes, you are right. I will implement it tomorrow.

@snoyberg
Copy link
Member

snoyberg commented Apr 3, 2015

This is now on Hackage, thanks! Can you confirm that the behavior is resolved and if so close the issue?

@kazu-yamamoto
Copy link
Contributor

LGTM.

@DeathByTape How about you?

@kazu-yamamoto
Copy link
Contributor

@snoyberg Where can I find ver-bumps-up commit for warp 3.0.11? It seems to me that the warp/3.0.11 tag is put on warp 3.0.10.1.

@snoyberg
Copy link
Member

snoyberg commented Apr 3, 2015

Good catch, commit pushed

On Fri, Apr 3, 2015, 6:46 AM Kazu Yamamoto notifications@github.com wrote:

@snoyberg https://github.com/snoyberg Where can I find ver-bumps-up
commit for warp 3.0.11? It seems to me that the warp/3.0.11 tag is put on
warp 3.0.10.1.


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

@kazu-yamamoto
Copy link
Contributor

Thanks. You should move tags.

@snoyberg
Copy link
Member

snoyberg commented Apr 3, 2015

Done.

On Fri, Apr 3, 2015 at 6:57 AM Kazu Yamamoto notifications@github.com
wrote:

Thanks. You should move tags.


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

@kazu-yamamoto
Copy link
Contributor

Thanks!

@DennisMcWherter
Copy link
Contributor Author

I promise I'm usually not this slow :) (read: busy week). In any case, I just pulled this off hackage and can confirm that it looks good. Good works guys! Thanks 👍

$ curl -4 -I -X GET http://localhost:3000/cabal.sandbox.config
HTTP/1.1 200 OK
Content-Length: 1034
Accept-Ranges: bytes
Date: Fri, 03 Apr 2015 11:55:58 GMT
Server: Warp/3.0.10.1
Content-Type: application/octet-stream
last-modified: Fri, 03 Apr 2015 11:43:21 GMT
$ curl -4 -I -X HEAD http://localhost:3000/cabal.sandbox.config
HTTP/1.1 200 OK
Content-Length: 1034
Accept-Ranges: bytes
Date: Fri, 03 Apr 2015 11:56:05 GMT
Server: Warp/3.0.10.1
Content-Type: application/octet-stream
last-modified: Fri, 03 Apr 2015 11:43:21 GMT

@snoyberg
Copy link
Member

snoyberg commented Apr 3, 2015

If this is you responding slowly, I'm afraid of fast ;). This response time
is certainly above average, no need to worry.

On Fri, Apr 3, 2015 at 2:56 PM Dennis McWherter notifications@github.com
wrote:

Closed #354 #354.


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

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

3 participants