JuliaWeb / HTTP.jl

HTTP for Julia
https://juliaweb.github.io/HTTP.jl/stable/
Other
635 stars 176 forks source link

fix: prevent `HEAD` requests from writing body in streamhandler #1113

Open pankgeorg opened 1 year ago

pankgeorg commented 1 year ago

(Note: if you're using your own streamhandler, you're on your own) fixes: https://github.com/JuliaWeb/HTTP.jl/issues/1112

with this fix, 3 connections fly over the same connection with cURL

curl -Ivvvvvvvvvvvvvvvv http://localhost:1234 http://localhost:1234 http://localhost:1234
*   Trying 127.0.0.1:1234...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 1234 (#0)
> HEAD / HTTP/1.1
> Host: localhost:1234
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
HTTP/1.1 200 OK

<
* Connection #0 to host localhost left intact
* Found bundle for host localhost: 0x563e0dc11eb0 [serially]
* Can not multiplex, even if we wanted to!
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (127.0.0.1) port 1234 (#0)
> HEAD / HTTP/1.1
> Host: localhost:1234
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
HTTP/1.1 200 OK

<
* Connection #0 to host localhost left intact
* Found bundle for host localhost: 0x563e0dc11eb0 [serially]
* Can not multiplex, even if we wanted to!
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (127.0.0.1) port 1234 (#0)
> HEAD / HTTP/1.1
> Host: localhost:1234
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
HTTP/1.1 200 OK

<
* Connection #0 to host localhost left intact
codecov-commenter commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (5cd586d) 82.70% compared to head (64d36e2) 82.71%. Report is 6 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1113 +/- ## ========================================== + Coverage 82.70% 82.71% +0.01% ========================================== Files 32 32 Lines 3053 3055 +2 ========================================== + Hits 2525 2527 +2 Misses 528 528 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fredrikekre commented 10 months ago

I tried seeing if there was somewhere else we could enforce this (like startwrite or unsafe_write for Stream)

@quinnj why did you dismiss this idea? Seems like we could bail early in https://github.com/JuliaWeb/HTTP.jl/blob/a2ce750aa28765d0bb6f905c41f11919173a3457/src/Streams.jl#L85 just like if n = 0? This would take care of both request handler and stream handler, I think?

pankgeorg commented 10 months ago

I tried seeing if there was somewhere else we could enforce this (like startwrite or unsafe_write for Stream)

@quinnj why did you dismiss this idea? Seems like we could bail early in

https://github.com/JuliaWeb/HTTP.jl/blob/a2ce750aa28765d0bb6f905c41f11919173a3457/src/Streams.jl#L85

just like if n = 0? This would take care of both request handler and stream handler, I think?

n = 0 is totally stateless and always correct to be a no-op, while the check for HEAD also needs to know that the headers are already written and the current bytes to write are part of the body (and not the headers).