Fix 16k file bucket #143
Fix 16k file bucket #143
Conversation
|
It looks like HTTP/gocloud buckets don't care if your byte range request goes past the end of the file? To match that behavior, maybe we could just remove the Also make sure you update mockBucket, and you can remove the extra logic I added to fakeArchive to pad if <16kb. |
It should be OK for files, however on HTTP we need to special-case it - right now it's caught by If we are treating ETags correctly and the server correctly implements conditional requests we should never see a 416, only 412s when etags are actually expired. The only case where we see a 416 is the <16kb case, and we don't even get the N successfully read bytes. So our HTTP bucket should probably detect 416 and retry. A bit ugly... |
|
I didn't think it was an etag thing, I meant that HTTP server range requests return the overlap of the requested range and the actual data range. I've tried on a few different HTTP servers and when you request from within the file past the end of it, it always returns a 206. 416 is only when the start of the range is > the file length. Are you saying that caddy returns that error when backed by an HTTP bucket or file bucket? The 416 error code spec indicates that it should only be thrown when none of the ranges "overlap the current extent" |
bdon
force-pushed
the
fix-16k-file-bucket
branch
from
0567099 to
e0f80f9
Compare
|
@msbarry ok, updated |
| @@ -60,11 +60,15 @@ func (m mockBucket) NewRangeReaderEtag(_ context.Context, key string, offset int | |||
| if len(etag) > 0 && resultEtag != etag { | |||
| return nil, "", 412, &RefreshRequiredError{} | |||
| } | |||
| if offset+length > int64(len(bs)) { | |||
| if offset > int64(len(bs)) { | |||
@msbarry
currently archives less than 16K don't work with file buckets (maybe same for HTTP depending on server)
The initial IO is for 16384 bytes to save roundtrips over the network. This is a useful optimization, and it doesn't seem worth special-casing non-network archives to do something different (do one IO for a few bytes to then get the exact length...)
However doing a
ReadAtfor 16384 bytes will read the entire archive and then raise an EOF error. This PR special cases range requests at offset 0 to allow shorter responses. I don't love this because it adds hidden hardcoded behavior based on offset=0.Alternative: add another boolean parameter to NewRangeReaderEtag that's like
allowShortResponse boolto make this behavior explicit. Thoughts?