shade: stop copying each upload chunk into a second heap buffer
WriteChunk copied the whole chunk, which lib/multipart already hands over in a buffer from the global memory pool, into a bytes.Buffer so that retries could re-send it. That doubled the per-part memory and made a fresh chunk-sized heap allocation (64 MiB by default) for every part, times the upload concurrency. The chunk reader is seekable, so find its size with Seek and rewind it inside the pacer closure instead, sending the pooled buffer directly. Also fix the error for a part that fails to upload, which formatted the buffer instead of the part number.
This commit is contained in:
+12
-11
@@ -3,7 +3,6 @@
|
||||
package shade
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"fmt"
|
||||
"io"
|
||||
@@ -155,17 +154,15 @@ func (s *shadeChunkWriter) WriteChunk(ctx context.Context, chunkNumber int, read
|
||||
return 0, err
|
||||
}
|
||||
|
||||
// Read chunk
|
||||
var chunk bytes.Buffer
|
||||
n, err := io.Copy(&chunk, reader)
|
||||
// Find the chunk size
|
||||
n, err := reader.Seek(0, io.SeekEnd)
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("failed to read chunk: %w", err)
|
||||
}
|
||||
|
||||
if n == 0 {
|
||||
return 0, nil
|
||||
}
|
||||
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("failed to read chunk: %w", err)
|
||||
}
|
||||
// Get presigned URL for this part
|
||||
var partURL api.PartURL
|
||||
|
||||
@@ -201,14 +198,18 @@ func (s *shadeChunkWriter) WriteChunk(ctx context.Context, chunkNumber int, read
|
||||
}
|
||||
|
||||
err = s.f.pacer.Call(func() (bool, error) {
|
||||
// Use a fresh reader for each attempt so retries resend the whole chunk
|
||||
opts.Body = bytes.NewReader(chunk.Bytes())
|
||||
// Rewind the chunk so each attempt sends it in full
|
||||
_, err = reader.Seek(0, io.SeekStart)
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
opts.Body = reader
|
||||
uploadRes, err = s.f.srv.Call(ctx, &opts)
|
||||
return shouldRetry(ctx, uploadRes, err)
|
||||
})
|
||||
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("failed to upload part %d: %w", chunk, err)
|
||||
return 0, fmt.Errorf("failed to upload part %d: %w", chunkNumber+1, err)
|
||||
}
|
||||
|
||||
if uploadRes.StatusCode != http.StatusOK && uploadRes.StatusCode != http.StatusCreated {
|
||||
|
||||
@@ -0,0 +1,65 @@
|
||||
package shade
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/rclone/rclone/fs"
|
||||
"github.com/rclone/rclone/lib/pacer"
|
||||
"github.com/rclone/rclone/lib/rest"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// newTestChunkWriter returns a chunk writer pointed at server, which must
|
||||
// answer the part URL request with a URL back to itself and the part PUT.
|
||||
func newTestChunkWriter(t *testing.T, server *httptest.Server) *shadeChunkWriter {
|
||||
ctx := context.Background()
|
||||
f := &Fs{
|
||||
srv: rest.NewClient(server.Client()),
|
||||
endpoint: server.URL,
|
||||
drive: "drive",
|
||||
pacer: fs.NewPacer(ctx, pacer.NewDefault(pacer.MinSleep(time.Millisecond), pacer.MaxSleep(time.Millisecond))),
|
||||
token: "token",
|
||||
tokenExp: time.Now().Add(time.Hour),
|
||||
}
|
||||
return &shadeChunkWriter{initToken: "init", f: f}
|
||||
}
|
||||
|
||||
// partServer returns a test server which hands out a part URL pointing at
|
||||
// itself and passes part PUTs to put.
|
||||
func partServer(t *testing.T, put func(w http.ResponseWriter, body []byte)) *httptest.Server {
|
||||
var server *httptest.Server
|
||||
server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if strings.Contains(r.URL.Path, "/upload/multipart/part/") {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
_, _ = fmt.Fprintf(w, `{"url":%q}`, server.URL+"/put")
|
||||
return
|
||||
}
|
||||
body, err := io.ReadAll(r.Body)
|
||||
require.NoError(t, err)
|
||||
put(w, body)
|
||||
}))
|
||||
return server
|
||||
}
|
||||
|
||||
// TestWriteChunkError checks that a part which keeps failing reports the
|
||||
// part number in the error.
|
||||
func TestWriteChunkError(t *testing.T) {
|
||||
server := partServer(t, func(w http.ResponseWriter, body []byte) {
|
||||
w.WriteHeader(http.StatusInternalServerError)
|
||||
})
|
||||
defer server.Close()
|
||||
|
||||
s := newTestChunkWriter(t, server)
|
||||
_, err := s.WriteChunk(context.Background(), 2, bytes.NewReader([]byte("shade")))
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "failed to upload part 3:")
|
||||
}
|
||||
Reference in New Issue
Block a user