tq: decrement uploaded bytes in basic_upload before retry #1958
Conversation
tools/iotools.go
Outdated
| return | ||
| } | ||
|
|
||
| // Seek wraps the underlying Seeker's "Seak" method, atomically updating the |
tq/basic_upload.go
Outdated
| var reader lfsapi.ReadSeekCloser = progress.NewBodyWithCallback(f, t.Size, ccb) | ||
|
|
||
| fcount := tools.NewCountingReadSeekCloser(f, stat.Size()) | ||
| var reader lfsapi.ReadSeekCloser = progress.NewBodyWithCallback(fcount, t.Size, ccb) |
There was a problem hiding this comment.
Why not just do the counting in the *bodyWithCallback that's returned? Why is a new type needed?
tools/iotools.go
Outdated
| atomic.AddInt64(&r.n, offset) | ||
| case io.SeekEnd: | ||
| atomic.SwapInt64(&r.n, r.totalSize+offset) | ||
| } |
There was a problem hiding this comment.
Does this really need to use atomic? It looks like the reader gets passed in through an http.Request body, which is not accessed from multiple goroutines.
tq/basic_upload.go
Outdated
| // read _so far_, so that the next iteration doesn't re-transfer | ||
| // those bytes, according to the progress meter. | ||
| offset := fcount.N() | ||
| ccb(t.Size, offset, -int(offset)) |
There was a problem hiding this comment.
If this was using the progress package's *bodyWithCallback, it could just be a single method like ResetProgress().
tq/basic_upload.go
Outdated
| } | ||
| var reader lfsapi.ReadSeekCloser = progress.NewBodyWithCallback(f, t.Size, ccb) | ||
|
|
||
| fcount := tools.NewCountingReadSeekCloser(f, stat.Size()) |
There was a problem hiding this comment.
Wouldn't t.Size work here too?
ttaylorr
force-pushed
the
upload-retries
branch
from
e27ce28 to
589dffb
Compare
Good question, and great call. I originally considered implementing this functionality on the wrapped callback reader that gets created, but that only happens conditionally, so I think I skipped over the body callback reader. I went ahead and re-implemented this on the callback reader in 589dffb. |
progress/copycallback.go
Outdated
| } | ||
|
|
||
| func NewByteBodyWithCallback(by []byte, totalSize int64, cb CopyCallback) ReadSeekCloser { | ||
| var _ ReadSeekCloser = (*BodyWithCallback)(nil) |
There was a problem hiding this comment.
These declarations are unnecessary. It wouldn't compile if this wasn't the case.
This pull-request fixes a bug that caused the progress meter to display that it had transferred more bytes than were included in the total push.
To set things up, I'm running a basic LFS server on my laptop that is configured to return an HTTP 400 when a client tries to upload more than 10 bytes. I've configured my repo to use that remote, and to push 502 files, the first 501 are less than 10 bytes.
The push is comprised of:
LFS uploads the first 1.85 KB successfully, and then tries 100 times (unsuccessfully) to upload the last file, but fails to decrement before it retries again, which is why the progress meter goes "over" the total count.
A more robust fix would expand the
tq.Adapterinterface to provide a flexible way for all transfer adapter implementations to report if they "lost" progress, but I think that fixing the most common case with relatively low effort is a good first step.I originally wanted to call
Seek(0, io.SeekCurrent)on the open file descriptor, but I decided against this for two reasons:io.Seeker, which may not always be the case.Instead, I wrote a new type in the
toolspackage, to implementio.ReadSeeker(andio.Closer, for usage with thelfsapipackage) which atomically keeps track of the number of bytes read. Importantly, it also "overrides" theSeek()method, so any sort of seeking that we do will still yield correct updates to the total number of bytes read.In conclusion:
works as expected.
/cc @git-lfs/core