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(
+	conn := newConn(tcpConn, log, nil, nil, cfg, nil)
-		context.Background(), 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.