diff --git a/modules/storage/minio.go b/modules/storage/minio.go index bf51a1642a..424000a0d8 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -209,7 +209,7 @@ func (m *MinioStorage) Save(path string, r io.Reader, size int64) (int64, error) m.ctx, m.bucket, m.buildMinioPath(path), - r, + io.NopCloser(r), // prevent minio from closing the reader size, minio.PutObjectOptions{ ContentType: "application/octet-stream", diff --git a/modules/storage/storage_test.go b/modules/storage/storage_test.go index af3dd9520e..76589d941a 100644 --- a/modules/storage/storage_test.go +++ b/modules/storage/storage_test.go @@ -5,6 +5,7 @@ package storage import ( "bytes" + "io" "testing" "forgejo.org/modules/setting" @@ -13,22 +14,39 @@ import ( "github.com/stretchr/testify/require" ) +type spyCloser struct { + io.Reader + closed int +} + +func (s *spyCloser) Close() error { + s.closed++ + return nil +} + +var _ io.ReadCloser = &spyCloser{} + func testStorageIterator(t *testing.T, typStr Type, cfg *setting.Storage) { l, err := NewStorage(typStr, cfg) require.NoError(t, err) - testFiles := [][]string{ - {"a/1.txt", "a1"}, - {"/a/1.txt", "aa1"}, // same as above, but with leading slash that will be trim - {"ab/1.txt", "ab1"}, - {"b/1.txt", "b1"}, - {"b/2.txt", "b2"}, - {"b/3.txt", "b3"}, - {"b/x 4.txt", "bx4"}, + testFiles := []struct { + path, content string + size int64 + }{ + {"a/1.txt", "a1", -1}, + {"/a/1.txt", "aa1", -1}, // same as above, but with leading slash that will be trim + {"ab/1.txt", "ab1", 3}, + {"b/1.txt", "b1", 2}, // minio closes when the size is set + {"b/2.txt", "b2", -1}, + {"b/3.txt", "b3", -1}, + {"b/x 4.txt", "bx4", -1}, } for _, f := range testFiles { - _, err = l.Save(f[0], bytes.NewBufferString(f[1]), -1) + sc := &spyCloser{bytes.NewBufferString(f.content), 0} + _, err = l.Save(f.path, sc, f.size) require.NoError(t, err) + assert.Equal(t, 0, sc.closed) } expectedList := map[string][]string{ diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index 9a7fd03aa8..117fc6ab88 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -399,7 +399,12 @@ func EndUploadBlob(ctx *context.Context) { } return } - defer uploader.Close() + doClose := true + defer func() { + if doClose { + uploader.Close() + } + }() if ctx.Req.Body != nil { if err := uploader.Append(ctx, ctx.Req.Body); err != nil { @@ -432,10 +437,11 @@ func EndUploadBlob(ctx *context.Context) { return } - // There was a strange bug: the "Close" fails with error "close .../tmp/package-upload/....: file already closed" - // AFAIK there should be no other "Close" call to the uploader between NewBlobUploader and this line. - // At least it's safe to call Close twice, so ignore the error. - _ = uploader.Close() + doClose = false + if err := uploader.Close(); err != nil { + apiError(ctx, http.StatusInternalServerError, err) + return + } if err := container_service.RemoveBlobUploadByID(ctx, uploader.ID); err != nil { apiError(ctx, http.StatusInternalServerError, err)