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

@@ -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 {

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 {