Repository navigation
feat(b2): add a progress callback for uploads and downloads - #131
ffumero2003 wants to merge 1 commit into
Conversation
jakub-zytka-reef
left a comment
jakub-zytka-reef
left a comment
There was a problem hiding this comment.
I am really confused by this PR.
The issue description says:
Every transferred byte already flows through one primitive, meteredReader (b2/writer.go:720), for both upload paths and downloads, and parts are marked done at completeChunk — so a callback can be fired from the existing plumbing without touching the concurrent-upload goroutine model.
This makes sense - let's update the progress as the bytes flow.
But then the PR updates progress only on full chunk download/upload. This is by default 10MB on download, 100MB on upload.
So if I'm uploading 80MB files the progress is essentially either 0% or 100%.
I don't have full business context regarding this feature, but this really doesn't seem right.
| } | ||
| cnk.buf.Close() | ||
| w.completeChunk(cnk.id) | ||
| w.completeChunk(cnk.id, completedBytes(cnk.buf)) |
There was a problem hiding this comment.
does that really work?
Doesn't cnk.buf.Close sets the buf to nil, so the completedBytes will panic?
Is there at test that covers this?
| } | ||
| w.progressMu.Lock() | ||
| defer w.progressMu.Unlock() | ||
| completed := atomic.AddInt64(&w.progressCompleted, bytesComplete) |
There was a problem hiding this comment.
IIRC go will panic if atomic instructions are used on non-8-bytes aligned fields.
This is unlikely to happen (raspberry pi? ancient PC?)
As I see it - atomics are not needed currently:
progressCompletedis only used under the lock (i.e. no need for atomic)progressTotalis not set concurrently with reads (this would only be a problem on 32bit anyway)
Or perhaps we could use atomic.Int64 which are aligned correctly?
| defer w.progressMu.Unlock() | ||
| completed := atomic.AddInt64(&w.progressCompleted, bytesComplete) | ||
| if w.progress != nil { | ||
| w.progress(completed, atomic.LoadInt64(&w.progressTotal)) |
There was a problem hiding this comment.
I feel uneasy about running an arbitrary callback under a lock. Additionally, reportProgress can be called from within Close, which also keeps some lock.
It may be tricky to avoid deadlocks etc.
Would it perhaps be better to use a 1-element channel instead?
| } | ||
| r.progressMu.Lock() | ||
| defer r.progressMu.Unlock() | ||
| completed := atomic.AddInt64(&r.progressCompleted, bytesComplete) |
There was a problem hiding this comment.
same as in the writer - atomic operations under lock on potentially unaligned data
| defer r.progressMu.Unlock() | ||
| completed := atomic.AddInt64(&r.progressCompleted, bytesComplete) | ||
| if r.Progress != nil { | ||
| r.Progress(completed, atomic.LoadInt64(&r.progressTotal)) |
There was a problem hiding this comment.
please see the comment in the writer
| if err != nil { | ||
| return 0, err | ||
| } | ||
| atomic.StoreInt64(&w.progressTotal, size) |
There was a problem hiding this comment.
why do we set progressTotal only here?
What if we are resuming?
Are there perhaps other scenarios where we can set total up front, like when we know the file size?
This branch has not been deployed
2 participants
Context
blazer reported transfer progress only by polling
Client.Status()(b2/monitor.go) — per-chunk ratios, no callback, no running byte total, no terminal event. The peer SDKs expose a progress listener, soclient.progressread partial.Changes
WithProgress(cb func(bytesComplete, bytesTotal int64)) WriterOptionfor uploads (mirrorsWithLegalHold), and a settableReader.Progressfield for downloads (no constructor signature change).atomic.AddInt64and delivery serialized under a mutex (may be invoked from an upload/download worker goroutine — documented).bytesTotalis the known size forReadFromof anio.Seeker(upload) or a finiteNewRangeReaderlength (download), and -1 when unknown (plainWrite, unbound streams, open-ended download).meteredReader/completeChunkplumbing;Client.Status()and the pull model are untouched (additive).Verification
go build ./...— clean.go test ./base/... ./b2/... -race— green, incl. new in-process callback tests (upload + download: fires, monotonic, ends at total).golangci-lint run --new-from-rev=origin/main ./...— 0 issues.client.progress@simulator — PASS (the test was rewritten from a pull-only reflection guard to a callback assertion: fires, monotonic, final == size).Note
Shares
b2/writer.gowith the SSE-C / SSE-B2 / compose branches, so whichever merges after the others needs routine conflict resolution.Closes #130