fix: transfer timeouts and nested directory creation - #79
Merged
Merged
Conversation
Config.DefaultTimeout lands in http.Client.Timeout, which is an absolute deadline covering the whole request including the body. That is right for the small JSON calls the API is mostly made of, but it also applied to streamed transfers: with the 30s default, every upload or download slower than 30 seconds was aborted mid-flight, no matter how generous a deadline the caller had put on ctx. uploadFileMultipart already worked around this with a private client that carries no Timeout. Lift that into Client.transferClient and a transferContext helper, and use both from uploadFileSingle and DownloadFileToPath, so every path that streams a body is bounded by the context instead of the wall clock. Callers who set their own deadline keep it; those who do not fall back to 30 minutes, as chunked uploads already did.
The API creates one directory level per request, so CreateDir could not create "newDir/subDir/anotherDir", which was the long-standing todo on it. Add CreateDirAll, which walks the path from the shallowest segment to the deepest and issues one request per missing level, mirroring os.MkdirAll: existing directories are left alone rather than reported as errors. Telling "already exists" apart from a real conflict needs the HTTP status, which ErrorResponse did not carry, so record it in a new StatusCode field (json:"-", filled in by this package) and expose the check as ErrorResponse.AlreadyExists. CreateDir keeps its single-level semantics, matching os.Mkdir.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two bugs surfaced while wiring this package into a TUI client.
Uploads and downloads died after 30 seconds
Config.DefaultTimeoutis assigned tohttp.Client.Timeout, an absolutedeadline over the entire request including the response body. Streamed
transfers ran on that client, so the 30s default killed any transfer that
took longer, roughly anything over 35 MB on a 10 Mbit/s link. A caller
passing a 30-minute context saw no benefit: whichever deadline fires first
wins, and it was always the client's.
uploadFileMultipartalready had the right fix locally, with a commentdescribing exactly this hazard. It just never reached
uploadFileSingleor the download path, and since
UploadFileFromPathWithProgressleavesChunkSizeat 0, the common upload path was always the unfixed one.CreateDircould not create nested pathsThe Yandex Disk API creates a single level per request, so
CreateDirfailed on
a/b/cunless every parent already existed. This was tracked bya
todoon the function.What changed
Client.transferClient()returns a client sharing the configuredtransport but carrying no
Timeout;transferContext()applies a30-minute fallback only when the caller set no deadline of their own.
Both are now used by single upload, multipart upload and download.
CreateDirAllcreates missing parents, mirroringos.MkdirAllandtreating existing directories as success.
CreateDiris unchanged andkeeps
os.Mkdirsemantics.ErrorResponse.StatusCoderecords the HTTP status behind an error(
json:"-", populated by this package), andErrorResponse.AlreadyExists()distinguishes a "directory is already there" conflict from a real one.
Compatibility
No breaking changes. Signatures are untouched;
StatusCodeis a new fieldon an existing struct and
CreateDirAll/AlreadyExistsare additions.Suitable for a patch or minor release.
Tests
transfer_timeout_test.godrives realhttptestservers that streambodies more slowly than
DefaultTimeout. Both transfer tests wereconfirmed to fail against the previous code with
context deadline exceeded (Client.Timeout ...)and to pass after thefix; a third test checks that a caller's own short deadline still aborts
the transfer.
createdirall_test.gocovers segment-by-segment creation, existingparents, idempotency, path forms (
disk:prefix, relative, redundantslashes), propagation of genuine errors, and rejection of traversal and
empty paths.
go test -race ./...passes.