fix: avoid logging error when server stop is requested (#571)

* fix: avoid logging error when server stop is requested

Fixes issue #498 where an error was logged when Stop() was called on the
FTP server. The handleAcceptError method now checks for 'use of closed
network connection' errors before logging, preventing unnecessary error
logs during normal shutdown.

- Move closed connection check before error logging in handleAcceptError
- Add test to verify no error is logged during normal server stop

Co-authored-by: Florent Clairambault <[email protected]>

* fix: resolve linter issues across codebase

- Fix errcheck issues by explicitly ignoring return values from disconnect() calls
- Replace net.DialTimeout with context-aware dialer.DialContext
- Replace net.Listen with net.ListenConfig.Listen using context
- Fix unused parameter warnings by renaming to underscore
- Remove unused nolint directive
- Fix variable name length issue (wg -> waitGroup)

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

* test: use testing.T context instead of context.Background()

Replace context.Background() with t.Context() in test files for proper
test lifecycle management and better cancellation behavior.

Co-authored-by: Florent Clairambault <[email protected]>

* fix: correct misspelling of errUnknowHash to errUnknownHash

Co-authored-by: Florent Clairambault <[email protected]>

* refactor: modernize Go code with current best practices

- Replace interface{} with any type alias (Go 1.18+)
- Use range over integer for count-based loops (Go 1.22+)
- Replace custom loop with slices.Contains
- Remove unnecessary variable copying in test loops
- Import slices package for slice utilities

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

* fix: remove unused context imports

---------

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: Florent Clairambault <[email protected]>
Co-authored-by: Claude <[email protected]>

Author: 3 people

client_handler_test.go

@@ -3,6 +3,7 @@ package ftpserver
 import (
 	"fmt"
 	"net"
+	"slices"
 	"sync"
 	"testing"
 	"time"
@@ -20,7 +21,7 @@ func TestConcurrency(t *testing.T) {
 	waitGroup := sync.WaitGroup{}
 	waitGroup.Add(nbClients)
 
-	for i := 0; i < nbClients; i++ {
+	for range nbClients {
 		go func() {
 			conf := goftp.Config{
 				User:     authUser,
@@ -47,7 +48,8 @@ func TestConcurrency(t *testing.T) {
 
 func TestDOS(t *testing.T) {
 	server := NewTestServer(t, true)
-	conn, err := net.DialTimeout("tcp", server.Addr(), 5*time.Second)
+	dialer := &net.Dialer{Timeout: 5 * time.Second}
+	conn, err := dialer.DialContext(t.Context(), "tcp", server.Addr())
 	require.NoError(t, err)
 
 	defer func() {
@@ -149,7 +151,8 @@ func TestConnectionNotAllowed(t *testing.T) {
 	}
 	s := NewTestServerWithTestDriver(t, driver)
 
-	conn, err := net.DialTimeout("tcp", s.Addr(), 5*time.Second)
+	dialer := &net.Dialer{Timeout: 5 * time.Second}
+	conn, err := dialer.DialContext(t.Context(), "tcp", s.Addr())
 	require.NoError(t, err)
 
 	defer func() {
@@ -315,13 +318,7 @@ second line
 }
 
 func isStringInSlice(s string, list []string) bool {
-	for _, c := range list {
-		if s == c {
-			return true
-		}
-	}
-
-	return false
+	return slices.Contains(list, s)
 }
 
 func TestUnknownCommand(t *testing.T) {
@@ -486,7 +483,7 @@ func TestExtraData(t *testing.T) {
 	require.Len(t, info, 1)
 
 	for k, v := range info {
-		ccInfo, ok := v.(map[string]interface{})
+		ccInfo, ok := v.(map[string]any)
 		require.True(t, ok)
 		extra, ok := ccInfo["extra"].(uint32)
 		require.True(t, ok)

handle_auth.go

@@ -11,15 +11,15 @@ func (c *clientHandler) handleUSER(user string) error {
 		err := verifier.PreAuthUser(c, user)
 		if err != nil {
 			c.writeMessage(StatusNotLoggedIn, fmt.Sprintf("User rejected: %v", err))
-			c.disconnect()
+			_ = c.disconnect()
 
 			return nil
 		}
 	}
 
 	if c.isTLSRequired() && !c.HasTLSForControl() {
 		c.writeMessage(StatusServiceNotAvailable, "TLS is required")
-		c.disconnect()
+		_ = c.disconnect()
 
 		return nil
 	}
@@ -52,7 +52,7 @@ func (c *clientHandler) handleUserTLS(user string) bool {
 	driver, err := verifier.VerifyConnection(c, user, tlsConn)
 	if err != nil {
 		c.writeMessage(StatusNotLoggedIn, fmt.Sprintf("TLS verification failed: %v", err))
-		c.disconnect()
+		_ = c.disconnect()
 
 		return true
 	}
@@ -82,14 +82,14 @@ func (c *clientHandler) handlePASS(param string) error {
 	switch {
 	case err == nil && c.driver == nil:
 		c.writeMessage(StatusNotLoggedIn, "Unexpected exception (driver is nil)")
-		c.disconnect()
+		_ = c.disconnect()
 	case err != nil:
 		if msg == "" {
 			msg = fmt.Sprintf("Authentication error: %v", err)
 		}
 
 		c.writeMessage(StatusNotLoggedIn, msg)
-		c.disconnect()
+		_ = c.disconnect()
 	default: // err == nil && c.driver != nil
 		if msg == "" {
 			msg = "Password ok, continue"

handle_auth_test.go

@@ -19,7 +19,8 @@ func panicOnError(err error) {
 func TestLoginSuccess(t *testing.T) {
 	server := NewTestServer(t, false)
 	// send a NOOP before the login, this doesn't seems possible using secsy/goftp so use the old way ...
-	conn, err := net.DialTimeout("tcp", server.Addr(), 5*time.Second)
+	dialer := &net.Dialer{Timeout: 5 * time.Second}
+	conn, err := dialer.DialContext(t.Context(), "tcp", server.Addr())
 	require.NoError(t, err)
 
 	defer func() {

handle_dirs_test.go

@@ -460,7 +460,8 @@ func TestPerClientTLSTransfer(t *testing.T) {
 
 func TestDirListingBeforeLogin(t *testing.T) {
 	s := NewTestServer(t, false)
-	conn, err := net.DialTimeout("tcp", s.Addr(), 5*time.Second)
+	dialer := &net.Dialer{Timeout: 5 * time.Second}
+	conn, err := dialer.DialContext(t.Context(), "tcp", s.Addr())
 	require.NoError(t, err)
 
 	defer func() {

handle_files.go

@@ -689,7 +689,7 @@ func (c *clientHandler) computeHashForFile(filePath string, algo HASHAlgo, start
 	case HASHAlgoSHA512:
 		chosenHashAlgo = sha512.New()
 	default:
-		return "", errUnknowHash
+		return "", errUnknownHash
 	}
 
 	file, err = c.getFileHandle(filePath, os.O_RDONLY, start)

handle_misc.go

@@ -10,7 +10,7 @@ import (
 	"time"
 )
 
-var errUnknowHash = errors.New("unknown hash algorithm")
+var errUnknownHash = errors.New("unknown hash algorithm")
 
 func (c *clientHandler) handleAUTH(_ string) error {
 	if tlsConfig, err := c.server.driver.GetTLSConfig(); err == nil {
@@ -316,7 +316,7 @@ func (c *clientHandler) handleQUIT(_ string) error {
 	}
 
 	c.writeMessage(StatusClosingControlConn, msg)
-	c.disconnect()
+	_ = c.disconnect()
 	c.reader = nil
 
 	return nil

server.go

@@ -2,6 +2,7 @@
 package ftpserver
 
 import (
+	"context"
 	"crypto/tls"
 	"errors"
 	"fmt"
@@ -208,7 +209,8 @@ func (server *FtpServer) Listen() error {
 }
 
 func (server *FtpServer) createListener() (net.Listener, error) {
-	listener, err := net.Listen("tcp", server.settings.ListenAddr)
+	lc := &net.ListenConfig{}
+	listener, err := lc.Listen(context.Background(), "tcp", server.settings.ListenAddr)
 	if err != nil {
 		server.Logger.Error("cannot listen on main port", "err", err, "listenAddr", server.settings.ListenAddr)
 
@@ -266,8 +268,6 @@ func (server *FtpServer) Serve() error {
 // It returns a boolean indicating if the error should stop the server and the error itself or none if it's a standard
 // scenario (e.g. a closed listener)
 func (server *FtpServer) handleAcceptError(err error, tempDelay *time.Duration) (bool, error) {
-	server.Logger.Error("Serve error", "err", err)
-
 	if errOp := (&net.OpError{}); errors.As(err, &errOp) {
 		// This means we just closed the connection and it's OK
 		if errOp.Err.Error() == "use of closed network connection" {
@@ -277,6 +277,8 @@ func (server *FtpServer) handleAcceptError(err error, tempDelay *time.Duration)
 		}
 	}
 
+	server.Logger.Error("Serve error", "err", err)
+
 	// see https://github.com/golang/go/blob/4aa1efed4853ea067d665a952eee77c52faac774/src/net/http/server.go#L3046
 	// & https://github.com/fclairamb/ftpserverlib/pull/352#pullrequestreview-1077459896
 	// The temporaryError method should replace net.Error.Temporary() when the go team

server_stop_test.go

@@ -0,0 +1,90 @@
+package ftpserver
+
+import (
+	"sync"
+	"testing"
+	"time"
+
+	log "github.com/fclairamb/go-log"
+	"github.com/stretchr/testify/require"
+)
+
+// TestServerStopDoesNotLogError tests that stopping a server doesn't log an error
+// when the listener is closed as expected
+func TestServerStopDoesNotLogError(t *testing.T) {
+	req := require.New(t)
+
+	// Create a server with a test driver
+	server := NewFtpServer(&TestServerDriver{
+		Settings: &Settings{
+			ListenAddr: "127.0.0.1:0", // Use dynamic port
+		},
+	})
+
+	// Use a custom logger that tracks error logs
+	mockLogger := &MockLogger{}
+	server.Logger = mockLogger
+
+	// Start listening
+	err := server.Listen()
+	req.NoError(err)
+
+	// Start serving in a goroutine
+	var serveErr error
+	var waitGroup sync.WaitGroup
+	waitGroup.Add(1)
+
+	go func() {
+		defer waitGroup.Done()
+		serveErr = server.Serve()
+	}()
+
+	// Give the server a moment to start accepting connections
+	time.Sleep(100 * time.Millisecond)
+
+	// Stop the server
+	err = server.Stop()
+	req.NoError(err)
+
+	// Wait for the Serve goroutine to finish
+	waitGroup.Wait()
+
+	// Serve should return nil (no error) when stopped normally
+	req.NoError(serveErr)
+
+	// Check that no error was logged for the "use of closed network connection"
+	// The mock logger should not have received any error logs
+	req.Empty(mockLogger.ErrorLogs, "Expected no error logs when stopping server, but got: %v", mockLogger.ErrorLogs)
+}
+
+// MockLogger captures log calls to verify behavior
+type MockLogger struct {
+	ErrorLogs []string
+	WarnLogs  []string
+	InfoLogs  []string
+	DebugLogs []string
+}
+
+func (m *MockLogger) Debug(message string, _ ...any) {
+	m.DebugLogs = append(m.DebugLogs, message)
+}
+
+func (m *MockLogger) Info(message string, _ ...any) {
+	m.InfoLogs = append(m.InfoLogs, message)
+}
+
+func (m *MockLogger) Warn(message string, _ ...any) {
+	m.WarnLogs = append(m.WarnLogs, message)
+}
+
+func (m *MockLogger) Error(message string, _ ...any) {
+	m.ErrorLogs = append(m.ErrorLogs, message)
+}
+
+func (m *MockLogger) Panic(message string, _ ...any) {
+	panic(message)
+}
+
+func (m *MockLogger) With(_ ...any) log.Logger {
+	return m
+}

server_test.go

@@ -84,7 +84,8 @@ func newFakeListener(err error) net.Listener {
 func TestCannotListen(t *testing.T) {
 	req := require.New(t)
 
-	portBlockerListener, err := net.Listen("tcp", "127.0.0.1:0")
+	lc := &net.ListenConfig{}
+	portBlockerListener, err := lc.Listen(t.Context(), "tcp", "127.0.0.1:0")
 	req.NoError(err)
 
 	defer func() { req.NoError(portBlockerListener.Close()) }()
@@ -107,7 +108,8 @@ func TestCannotListen(t *testing.T) {
 func TestListenWithBadTLSSettings(t *testing.T) {
 	req := require.New(t)
 
-	portBlockerListener, err := net.Listen("tcp", "127.0.0.1:0")
+	lc := &net.ListenConfig{}
+	portBlockerListener, err := lc.Listen(t.Context(), "tcp", "127.0.0.1:0")
 	req.NoError(err)
 
 	defer func() { req.NoError(portBlockerListener.Close()) }()
@@ -173,7 +175,6 @@ func TestQuoteDoubling(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			require.Equal(t, tt.want, quoteDoubling(tt.args.s))
 		})

transfer_active_test.go

@@ -26,7 +26,8 @@ func TestRemoteAddrFormat(t *testing.T) {
 }
 
 func TestActiveTransferFromPort20(t *testing.T) {
-	listener, err := net.Listen("tcp", ":20") //nolint:gosec
+	lc := &net.ListenConfig{}
+	listener, err := lc.Listen(t.Context(), "tcp", ":20")
 	if err != nil {
 		t.Skipf("Binding on port 20 is not supported here: %v", err)
 	}