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.