From ea9a64c751839f59f5b2be8f83080c6751529b61 Mon Sep 17 00:00:00 2001 From: alliasgher Date: Sun, 2 Aug 2026 22:50:21 +0500 Subject: [PATCH] smb: reuse the upload connection for SetModTime - fixes #9675 Object.Update held its connection until the deferred putConnection ran at function exit, so the SetModTime it does at the end of every upload had to take a second connection from the pool, dialling a whole new SMB session when the pool was empty. With N transfers in flight the pool grew to roughly 2N sessions for no reason. Return the connection as soon as the file is closed. At that point the upload has succeeded and remove() can no longer be reached, so nothing else needs it, and SetModTime picks the same connection straight back out of the pool. putConnection nils the pointer, so the deferred putConnection becomes a no-op and the connection is not returned twice. --- backend/smb/smb.go | 5 ++++ backend/smb/smb_internal_test.go | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) 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 {