Resolve IRC client hostnames off the accept loop (closes #156)

acceptLoop called newConn, which did the reverse DNS lookup, with its
three-second timeout, before the loop could accept the next
connection, so one client whose address had no answering reverse DNS
held up every other IRC connect. serve now does the lookup at its
start, on the connection's own goroutine, and the accept loop only
accepts.

The server keeps the lookup function in a field, as it does the ping
times, so that a test can give it one that blocks. The new test
checks that a second client registers while the first one's lookup
has not returned.

Model: opus-5-5

internal/ircserver/conn.go

@@ -70,7 +70,6 @@ type Conn struct {
 }
 
 func newConn(
-	ctx context.Context,
 	tcpConn net.Conn,
 	log *slog.Logger,
 	database *db.Database,
@@ -94,7 +93,6 @@ func newConn(
 		svc:       svc,
 		serverSfx: srvName,
 		remoteIP:  host,
-		hostname:  resolveHost(ctx, host),
 	}
 
 	conn.commands = conn.buildCommandMap()
@@ -212,12 +210,15 @@ func resolveHost(ctx context.Context, addr string) string {
 }
 
 // serve is the main loop for a single IRC client connection.
+// It first sets the client's hostname to what resolve returns
+// for the client's address.
 // Cancelling ctx when it returns stops the relay goroutine.
 // When nothing has been read for interval the client is sent
 // a PING, and when nothing is read within deadline after that
 // the connection is closed.
 func (c *Conn) serve(
 	ctx context.Context,
+	resolve func(ctx context.Context, addr string) string,
 	interval, deadline time.Duration,
 ) {
 	ctx, cancel := context.WithCancel(ctx)
@@ -226,6 +227,11 @@ func (c *Conn) serve(
 	// still end the session then.
 	defer c.cleanup(context.WithoutCancel(ctx))
 
+	// The hostname is looked up here, on this connection's
+	// goroutine, and not when the connection is accepted, so
+	// that a slow lookup holds up only this client.
+	c.hostname = resolve(ctx, c.remoteIP)
+
 	ping := time.AfterFunc(interval, func() {
 		c.send("PING :" + c.serverSfx)
 	})

internal/ircserver/export_test.go

@@ -15,7 +15,8 @@ import (
 
 // NewTestServer creates a Server suitable for testing, whose
 // connections use interval and deadline in place of
-// pingInterval and pongDeadline.
+// pingInterval and pongDeadline, and resolve in place of
+// resolveHost.
 // The caller must call Stop() when finished.
 func NewTestServer(
 	log *slog.Logger,
@@ -23,6 +24,7 @@ func NewTestServer(
 	database *db.Database,
 	brk *broker.Broker,
 	interval, deadline time.Duration,
+	resolve func(ctx context.Context, addr string) string,
 ) *Server {
 	globs := &globals.Globals{
 		Appname:   "neoirc",
@@ -44,9 +46,16 @@ func NewTestServer(
 
 		pingInterval: interval,
 		pongDeadline: deadline,
+		resolveHost:  resolve,
 	}
 }
 
+// ResolveHost is the reverse DNS lookup that a server made by
+// New does for each client.
+func ResolveHost(ctx context.Context, addr string) string {
+	return resolveHost(ctx, addr)
+}
+
 // Start exposes the unexported start method for tests.
 func (s *Server) Start(addr string) error {
 	return s.start(context.Background(), addr)
@@ -97,10 +106,7 @@ func NewTestConn(
 	tcpConn net.Conn,
 	nick string,
 ) *Conn {
-	conn := newConn(
-		context.Background(), tcpConn, log,
-		nil, nil, cfg, nil,
-	)
+	conn := newConn(tcpConn, log, nil, nil, cfg, nil)
 	conn.nick = nick
 
 	return conn

internal/ircserver/server.go

@@ -52,6 +52,10 @@ type Server struct {
 	// same names, except in tests, which shorten them.
 	pingInterval time.Duration
 	pongDeadline time.Duration
+
+	// resolveHost is the function of the same name, except in
+	// tests, which replace it with one that blocks.
+	resolveHost func(ctx context.Context, addr string) string
 }
 
 // New creates a new IRC Server and registers its lifecycle
@@ -75,6 +79,7 @@ func New(
 
 		pingInterval: pingInterval,
 		pongDeadline: pongDeadline,
+		resolveHost:  resolveHost,
 	}
 
 	listenAddr := params.Config.IRCListenAddr
@@ -175,7 +180,7 @@ func (s *Server) acceptLoop(ctx context.Context) {
 		}
 
 		client := newConn(
-			ctx, tcpConn, s.log,
+			tcpConn, s.log,
 			s.database, s.brk, s.cfg, s.svc,
 		)
 
@@ -204,7 +209,10 @@ func (s *Server) acceptLoop(ctx context.Context) {
 				s.mu.Unlock()
 			}()
 
-			client.serve(ctx, s.pingInterval, s.pongDeadline)
+			client.serve(
+				ctx, s.resolveHost,
+				s.pingInterval, s.pongDeadline,
+			)
 		}()
 	}
 }

internal/ircserver/server_test.go

@@ -2,6 +2,7 @@ package ircserver_test
 
 import (
 	"bufio"
+	"context"
 	"crypto/rand"
 	"database/sql"
 	"errors"
@@ -11,6 +12,7 @@ import (
 	"os"
 	"runtime"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -90,6 +92,21 @@ func newTestEnvWithPingTimes(
 ) *testEnv {
 	t.Helper()
 
+	return newTestEnvWithResolver(
+		t, cfg, interval, deadline, ircserver.ResolveHost,
+	)
+}
+
+// newTestEnvWithResolver is newTestEnvWithPingTimes, except
+// that the server looks up a client's hostname with resolve.
+func newTestEnvWithResolver(
+	t *testing.T,
+	cfg *config.Config,
+	interval, deadline time.Duration,
+	resolve func(ctx context.Context, addr string) string,
+) *testEnv {
+	t.Helper()
+
 	// A random name, so that no other test environment, not
 	// even an earlier run of the same test, can share it.
 	dsn := fmt.Sprintf(
@@ -147,7 +164,7 @@ func newTestEnvWithPingTimes(
 	))
 
 	srv := ircserver.NewTestServer(
-		log, cfg, database, brk, interval, deadline,
+		log, cfg, database, brk, interval, deadline, resolve,
 	)
 
 	err = srv.Start(addr)
@@ -415,6 +432,49 @@ func TestRegistration(t *testing.T) {
 	assertContains(t, lines, " 001 ", "RPL_WELCOME")
 }
 
+// TestRegistrationWhileAnotherLookupIsPending checks that a
+// client is accepted and registered while the hostname lookup
+// of an earlier client has not returned, and that it
+// registers with the hostname its own lookup returned.
+func TestRegistrationWhileAnotherLookupIsPending(t *testing.T) {
+	t.Parallel()
+
+	const lookedUpHost = "client.example"
+
+	var lookups atomic.Int32
+
+	firstLookupStarted := make(chan struct{})
+
+	// The first lookup returns only when the server stops.
+	resolve := func(ctx context.Context, _ string) string {
+		if lookups.Add(1) == 1 {
+			close(firstLookupStarted)
+			<-ctx.Done()
+		}
+
+		return lookedUpHost
+	}
+
+	env := newTestEnvWithResolver(
+		t, &config.Config{ServerName: testServerName},
+		ircserver.PingInterval, ircserver.PongDeadline, resolve,
+	)
+
+	env.dial(t)
+
+	select {
+	case <-firstLookupStarted:
+	case <-time.After(testTimeout):
+		t.Fatal("the first client's lookup did not start")
+	}
+
+	lines := env.dial(t).register("second")
+	assertContains(
+		t, lines, "second!second@"+lookedUpHost,
+		"RPL_WELCOME with the looked-up hostname",
+	)
+}
+
 // TestEachTestEnvHasItsOwnDatabase checks that a test
 // environment starts on an empty database while another one
 // is still open.