Closed Drvi closed 11 months ago
Attention: 1 lines
in your changes are missing coverage. Please review.
Comparison is base (
0229c4c
) 84.21% compared to head (6b52c0b
) 84.32%.
Files | Patch % | Lines |
---|---|---|
src/get.jl | 92.30% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks @quinnj! Would you prefer for the two error paths to report
a) What HTTP returns "Unable to grow response stream IOBuffer $(sizeof(out)) large enough for response body size: $(sizeof(in))"
or
b) What we currently do for multipart downloads "out ($(sizeof(out))) must at least be of length $(sizeof(in))"
?
Changing from a) to b) here would require some pattern matching on the message in the ArgumentError we intercepted from HTTP.jl, which I'm not the biggest fan of.
Yeah, I think going w/ a
is fine and easier, right?
There are two main codepaths for
get
-- we either do a multipart download for larger files or a singleGET
call for smaller ones. When we give the method a preallocated buffer, we also constrain the maximum size of the object we want to download (anything bigger than the buffer should fail). When the object is larger than the buffer, the error handling is different for the two codepaths:multipart download starts with a single
HEAD
call to learn the size of the object, and once we know it, we can compare it with the provided buffer, and throw anArgumentError
early.For small files, we don't do the extra
HEAD
request, but we letHTTP.jl
handle the case internally in the singleGET
call and if the buffer turned out to be too small, we'd get aRequestError
wrapping andArgumentError
back fromHTTP.jl
.This means that depending on the size of the input buffer, we'd sometimes get an
ArgumentError
and sometimes aRequestError
both communicating the same underlying problem -- the input buffer is too small.My current understanding is that to make these exception types more predictable, we always threw an additional
ArgumentError
in the small file case, assuming that buffer size mismatch was the only possible source of error at the call site, but we've seen that other kinds of errors are possible and that theArgumentError
was hiding the actual problem in those cases.I also think that we used to do a
HEAD
request even for small files in the past and when we stopped doing that for performance reasons, we forgot that theHEAD
was shielding theGET
from various errors like 403, 404 and so on.In this PR, we no longer throw an additional
ArgumentError
in the small file case, instead we inspect theRequestError
and if it contains a singleArgumentError
we rethrow that. This is done because it's backward compatible -- we wanted to throw anArgumentError
on buffer size mismatch before so we'll continue to do so. I'm not sure if this is the right call though, and would appreciate some feedback/discussion on that.