From c28a8817a48a175f614c70380ccfb143429e7ea6 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 21 Oct 2026 01:02:40 +0000 Subject: [PATCH] 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 | 10 ++++- internal/ircserver/export_test.go | 16 +++++--- internal/ircserver/server.go | 12 +++++- internal/ircserver/server_test.go | 62 ++++++++++++++++++++++++++++++- 4 files changed, 90 insertions(+), 10 deletions(-) diff --git a/internal/ircserver/conn.go b/internal/ircserver/conn.go index e11d09b..bf7c9f4 100644 --- a/internal/ircserver/conn.go +++ b/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) }) diff --git a/internal/ircserver/export_test.go b/internal/ircserver/export_test.go index 11f4b8d..69b8b61 100644 --- a/internal/ircserver/export_test.go +++ b/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 diff --git a/internal/ircserver/server.go b/internal/ircserver/server.go index 3459860..8269934 100644 --- a/internal/ircserver/server.go +++ b/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, + ) }() } } diff --git a/internal/ircserver/server_test.go b/internal/ircserver/server_test.go index 5cbe493..b5970af 100644 --- a/internal/ircserver/server_test.go +++ b/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. -- 2.54.0