Alternate Downloads.jl-based backend (alternative to HTTP.jl) #396
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| struct DownloadsBackend <: AWS.AbstractBackend | ||
| downloader::Union{Nothing, Downloads.Downloader} | ||
| end | ||
|
|
||
| DownloadsBackend() = DownloadsBackend(nothing) | ||
|
|
||
| const AWS_DOWNLOADER = Ref{Union{Nothing, Downloader}}(nothing) | ||
| const AWS_DOWNLOAD_LOCK = ReentrantLock() | ||
|
|
||
| # Here we mimic Download.jl's own setup for using a global downloader. | ||
| # We do this to have our own downloader (separate from Downloads.jl's global downloader) | ||
| # because we add a hook to avoid redirects in order to try to match the HTTPBackend's | ||
| # implementation, and we don't want to mutate the global downloader from Downloads.jl. | ||
| # https://github.com/JuliaLang/Downloads.jl/blob/84e948c02b8a0625552a764bf90f7d2ee97c949c/src/Downloads.jl#L293-L301 | ||
| function get_downloader(downloader=nothing) | ||
| lock(AWS_DOWNLOAD_LOCK) do | ||
| yield() # let other downloads finish | ||
| downloader isa Downloader && return | ||
| while true | ||
| downloader = AWS_DOWNLOADER[] | ||
| downloader isa Downloader && return | ||
| D = Downloader() | ||
| D.easy_hook = (easy, info) -> Curl.setopt(easy, Curl.CURLOPT_FOLLOWLOCATION, false) | ||
| AWS_DOWNLOADER[] = D | ||
| end | ||
| end | ||
| return downloader | ||
| end | ||
|
Comment on lines
+15
to
+28
Member
Author
There was a problem hiding this comment. We may want to use the formulation from JuliaLang/Downloads.jl#136 instead
Member
There was a problem hiding this comment. Perhaps we can leave it until it gets merged upstream |
||
|
|
||
| # https://github.com/JuliaWeb/HTTP.jl/blob/2a03ca76376162ffc3423ba7f15bd6d966edff9b/src/MessageRequest.jl#L84-L85 | ||
| body_length(x::AbstractVector{UInt8}) = length(x) | ||
| body_length(x::AbstractString) = sizeof(x) | ||
|
mattBrzezinski marked this conversation as resolved.
|
||
|
|
||
| read_body(x::IOBuffer) = take!(x) | ||
| function read_body(x::IO) | ||
| close(x) | ||
| return read(x) | ||
|
Contributor
There was a problem hiding this comment. Was just reading through this, I don't understand
Member
There was a problem hiding this comment. This was a workaround specifically for I'm no expert when it comes to the various
Contributor
There was a problem hiding this comment. I think perhaps we just need to use Perhaps for this case, the newly available — but strangely named — But even so, I'm not sure that AWS should be unilaterally closing or shutting down the write side of the the stream at all — to me that seems to be the business of the code which passes in the stream. For example, if reading from a pipe, presumably you want to keep reading until the writing side decides it's done? There was a problem hiding this comment. Agreed, seems like whoever was filling the buffer needs to call
Member
Author
There was a problem hiding this comment.
Well, we only do it if
I’m not sure who that is- Downloads.jl fills it but I don’t think they should be closing it (because of exactly what @c42f just said). There was a problem hiding this comment. Well, somebody was wrong before, and now AWS.jl is now possibly also very bad at stream EOF handling here too. Whoever created the stream is generally responsible for marking when it is done, or passing it to another function which will take care of that. I suspect Downloads.jl absolutely should be copying the EOF marker from the underlying stream also. It is out-of-band data, but likely just as relevant as the in-band bytes.
Contributor
There was a problem hiding this comment.
Perhaps I've misunderstood what's going on here, but is the problem in Downloads.jl then? Shouldn't they close the write side when writing is done?
Member
Author
There was a problem hiding this comment. I 100% don't understand what's going on. But let me try to summarize the current behavior (including this PR). SummaryThe HTTPBackend only sets a
If I'm understanding then, I think Case 2 is probematic for the DownloadsBackend, and we should just not try to return bytes in this case, matching the behavior of the HTTP backend. I.e. we should not be closing the stream in this case. Case 3: I don't really know if we should close the stream or not. I think ideally this case wouldn't exist, because I don't think there really should be a So: if that analysis is right, Downloads.jl is fine, and we have at least one problematic case here. But I'll admit to definitely not fully understanding the semantics of how streams should work (and the manual isn't very helpful in this regard), though the discussion here helps me a bit. Though possibly, HTTP.jl and Downloads.jl should be closing the streams whenever they are passed and the backend is done populating them? I think maybe that's what @vtjnash is saying in #396 (comment). In which case most/all of these cases are wrong for both backends. Appendix: Following the path of
|
||
| end | ||
|
|
||
| function _http_request(backend::DownloadsBackend, request) | ||
| # If we pass `output`, older versions of Downloads.jl will | ||
| # expect a message body in the response. Specifically, it sets | ||
| # <https://curl.se/libcurl/c/CURLOPT_NOBODY.html> | ||
| # only when we do not pass the `output` argument. | ||
| # See <https://github.com/JuliaLang/Downloads.jl/issues/131>. | ||
| # | ||
| # When the method is `HEAD`, the response may have a Content-Length | ||
| # but not send any content back (which appears to be correct, | ||
| # <https://stackoverflow.com/a/18925736/12486544>). | ||
| # | ||
| # Thus, if we did not set `CURLOPT_NOBODY`, and it gets a Content-Length | ||
| # back, it will hang waiting for that body. | ||
| # | ||
| # Therefore, we do not pass an `output` when the `request_method` is `HEAD`. | ||
| # (Note: this is fixed on the latest Downloads.jl, but we include this workaround | ||
| # for compatability). | ||
| if request.response_stream === nothing | ||
| request.response_stream = IOBuffer() | ||
| end | ||
| output_arg = if request.request_method == "HEAD" | ||
| NamedTuple() | ||
| else | ||
| (; output=request.response_stream) | ||
| end | ||
|
|
||
| # If we're going to return the stream, we don't want to read the body into an | ||
| # HTTP.Response we're never going to use. If we do that, the returned stream | ||
| # will have no data available (and reading from it could hang forever). | ||
| body_arg = if request.request_method == "HEAD" || request.return_stream | ||
| () -> NamedTuple() | ||
| else | ||
| () -> (; body = read_body(request.response_stream)) | ||
| end | ||
|
|
||
| # HTTP.jl sets this header automatically. | ||
| request.headers["Content-Length"] = string(body_length(request.content)) | ||
|
|
||
| # We pass an `input` only when we have content we wish to send. | ||
| input = IOBuffer() | ||
| input_arg = if !isempty(request.content) | ||
| write(input, request.content) | ||
| (; input=input) | ||
| else | ||
| NamedTuple() | ||
| end | ||
|
|
||
| @repeat 4 try | ||
| downloader = @something(backend.downloader, get_downloader()) | ||
| # set the hook so that we don't follow redirects. Only | ||
| # need to do this on per-request downloaders, because we | ||
| # set our global one with this hook already. | ||
| if backend.downloader !== nothing && downloader.easy_hook === nothing | ||
| downloader.easy_hook = (easy, info) -> Curl.setopt(easy, Curl.CURLOPT_FOLLOWLOCATION, false) | ||
| end | ||
|
|
||
| # We seekstart on every attempt, otherwise every attempt | ||
| # but the first will send an empty payload. | ||
| seekstart(input) | ||
|
|
||
| response = Downloads.request(request.url; input_arg..., output_arg..., | ||
| method = request.request_method, | ||
| headers = request.headers, verbose=false, throw=true, | ||
| downloader=downloader) | ||
|
|
||
| http_response = HTTP.Response(response.status, response.headers; body_arg()..., request=nothing) | ||
|
|
||
| if HTTP.iserror(http_response) | ||
| target = HTTP.resource(HTTP.URI(request.url)) | ||
| throw(HTTP.StatusError(http_response.status, request.request_method, target, http_response)) | ||
| end | ||
| return http_response | ||
| catch e | ||
| @delay_retry if ((isa(e, HTTP.StatusError) && AWS._http_status(e) >= 500) || isa(e, Downloads.RequestError)) end | ||
| end | ||
| end | ||
Uh oh!
There was an error while loading. Please reload this page.