diff --git a/backend/smb/smb.go b/backend/smb/smb.go index e858f86c4..c97a8e391 100644 --- a/backend/smb/smb.go +++ b/backend/smb/smb.go @@ -836,6 +836,11 @@ func (o *Object) Update(ctx context.Context, in io.Reader, src fs.ObjectInfo, op return fmt.Errorf("Update Close failed: %w", err) } + // Return the connection so the SetModTime below can reuse it rather than + // dialling a second one - the file is closed so remove() no longer needs + // it. This nils cn out, making the deferred putConnection a no-op. + o.fs.putConnection(&cn, nil) + // Set the modified time and also o.statResult err = o.SetModTime(ctx, src.ModTime(ctx)) if err != nil { diff --git a/backend/smb/smb_internal_test.go b/backend/smb/smb_internal_test.go index 46f348754..00642ead7 100644 --- a/backend/smb/smb_internal_test.go +++ b/backend/smb/smb_internal_test.go @@ -3,11 +3,18 @@ package smb import ( "context" + "errors" + "fmt" "io" "net" + "strings" "testing" "time" + "github.com/rclone/rclone/fs" + "github.com/rclone/rclone/fs/object" + "github.com/rclone/rclone/fstest" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -46,6 +53,46 @@ func TestDialClosesConnectionOnSetupError(t *testing.T) { require.ErrorIs(t, err, io.EOF) } +// TestUploadConnectionReuse checks an upload leaves only one connection in the +// pool, ie the connection it used is available again for the SetModTime which +// follows it rather than a second one being dialled. +// +// This needs a real SMB server so it is skipped if one isn't configured. +func TestUploadConnectionReuse(t *testing.T) { + ctx := context.Background() + fstest.Initialise() + remoteName := *fstest.RemoteName + if remoteName == "" { + remoteName = "TestSMB:rclone" + } + remote, err := fs.NewFs(ctx, remoteName) + if errors.Is(err, fs.ErrorNotFoundInConfigFile) { + t.Skipf("skipping as %q is not configured", remoteName) + } + require.NoError(t, err) + f, ok := remote.(*Fs) + if !ok { + t.Skipf("skipping as %q is not an SMB remote", remoteName) + } + + defer func() { require.NoError(t, f.Shutdown(ctx)) }() + + // Empty the pool so the connections counted below are only the upload's + require.NoError(t, f.drainPool(ctx)) + + const contents = "connection reuse test" + remotePath := fmt.Sprintf("rclone-test-connection-reuse-%d.txt", time.Now().UnixNano()) + src := object.NewStaticObjectInfo(remotePath, time.Now(), int64(len(contents)), true, nil, nil) + o, err := f.Put(ctx, strings.NewReader(contents), src) + require.NoError(t, err) + defer func() { require.NoError(t, o.Remove(ctx)) }() + + f.poolMu.Lock() + pooled := len(f.pool) + f.poolMu.Unlock() + assert.Equal(t, 1, pooled, "upload should leave exactly one connection in the pool") +} + // TestIsPathDir tests the isPathDir function logic func TestIsPathDir(t *testing.T) { tests := []struct {