Set CURLOPT_NOBODY automatically for HEADs?

[ericphanson]

In

Downloads.jl/src/Downloads.jl

Line 336 in 99165c6

set_body(easy, have_output)

we decide whether or not to set CURLOPT_NOBODY based on whether or not the user specifies an output. On the one hand, that makes sense, and lets the caller decide whether or not to set CURLOPT_NOBODY in a simple and intuitive way. On the other hand, HEAD responses should reply with a Content-Length if the GET would but should not return a body; if additionally CURLOPT_NOBODY is not set, this this will cause hangs. Thus, I wonder if Downloads.jl should set CURLOPT_NOBODY for HEADs automatically.

xref JuliaCloud/AWS.jl@a481a46

[StefanKarpinski]

Can you give a self-contained example of the problematic behavior?

[ericphanson]

Can you give a self-contained example of the problematic behavior?

julia> server = "https://httpbingo.julialang.org"
"https://httpbingo.julialang.org"

julia> output = IOBuffer()
IOBuffer(data=UInt8[...], readable=true, writable=true, seekable=true, append=false, size=0, maxsize=Inf, ptr=1, mark=-1)

julia> request(server * "/image/jpeg"; method="HEAD", output)
ERROR: HTTP/1.1 200 OK (Operation too slow. Less than 1 bytes/sec transferred the last 20 seconds) while requesting https://httpbingo.julialang.org/image/jpeg
Stacktrace:
 [1] (::Downloads.var"#9#18"{IOBuffer, Base.DevNull, String, Vector{Pair{String, String}}, Float64, Nothing, Bool, Bool, String, Int64, Bool, Bool})(easy::Downloads.Curl.Easy)
   @ Downloads /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.7/Downloads/src/Downloads.jl:369
 [2] with_handle(f::Downloads.var"#9#18"{IOBuffer, Base.DevNull, String, Vector{Pair{String, String}}, Float64, Nothing, Bool, Bool, String, Int64, Bool, Bool}, handle::Downloads.Curl.Easy)
   @ Downloads.Curl /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.7/Downloads/src/Curl/Curl.jl:64
 [3] #8
   @ /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.7/Downloads/src/Downloads.jl:311 [inlined]
 [4] arg_write(f::Downloads.var"#8#17"{Base.DevNull, String, Vector{Pair{String, String}}, Float64, Nothing, Bool, Bool, String, Int64, Bool, Bool}, arg::IOBuffer)
   @ ArgTools /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.7/ArgTools/src/ArgTools.jl:112
 [5] #7
   @ /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.7/Downloads/src/Downloads.jl:310 [inlined]
 [6] arg_read
   @ /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.7/ArgTools/src/ArgTools.jl:61 [inlined]
 [7] request(url::String; input::Nothing, output::IOBuffer, method::String, headers::Vector{Pair{String, String}}, timeout::Float64, progress::Nothing, verbose::Bool, throw::Bool, downloader::Nothing)
   @ Downloads /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.7/Downloads/src/Downloads.jl:309
 [8] top-level scope
   @ REPL[82]:1

[StefanKarpinski]

Ok, I see the issue. If you make a HEAD request and don't specify any output argument, then it works fine but if you make a HEAD request with an output argument then the request will hang. I guess the question is why are you making a HEAD request with an output argument, but I guess it would make sense to change this. Seems like changing that line to set_body(easy, have_output && method != "HEAD") would fix this issue. Then the other thing to do would be to add your test case to the tests. All the other tests seem to still pass for me with this. Want to make a pull request?

[ericphanson]

I guess the question is why are you making a HEAD request with an output argument

Yeah, I think that's a valid question. In my case, it was because AWS.jl decides what kind of request to make in one of a zillion autogenerated functions, and then passes that to a generic submit_request function which just translates it to HTTP.jl (currently) or Downloads.jl (my PR) and wraps it in retry loops etc. So that submit_request function previously was just always passing an output and assuming it will be empty when there is no body, leading to confusing errors (well, confusing for me, partly because whether or not you got the error depended on whether you called isfile first which issued a HEAD vs just s3_get, and I didn't notice the difference in the two code paths at first).

I updated my PR to inspect what kind of request we are making and choose whether or not to pass an output accordingly, which is a fine solution for AWS.jl. But I thought it was kind of a "gotcha" because it seems like it is never useful to wait for a body on a HEAD request (though if you really needed to, you could set it in easy_hook), and I thought this scenario of "generically" wrapping Downloads for some downstream task will probably come up again.

Seems like changing that line to set_body(easy, have_output && method != "HEAD") would fix this issue. Then the other thing to do would be to add your test case to the tests. All the other tests seem to still pass for me with this. Want to make a pull request?

Sure, I'll give it a go.

[StefanKarpinski]

I agree that it's best for us to try to automatically do what will work here. If someone wanted to override that behavior and unset the CURLOPT_NOBODY option, they could always use the easy_hook escape hatch.