PING a quiet IRC listener client before closing it (closes #155)

serve closed a connection after five minutes with nothing read and never
sent a PING, so a quiet registered client lost its session. It now sends
PING :<server> after pingInterval with nothing read, and closes the
connection when nothing is read within pongDeadline after that. Every
line read starts both waits again. readTimeout is removed.

Unregistered connections are sent the PING too, so a PONG before
registration is now ignored instead of answered with ERR_NOTREGISTERED.

Model: opus-5-5

internal/ircserver/conn.go

@@ -45,11 +45,6 @@ type Conn struct {
 	serverSfx string
 	commands  map[string]cmdHandler
 
-	// pingInterval and pongDeadline are the constants of the
-	// same names, except in tests, which shorten them.
-	pingInterval time.Duration
-	pongDeadline time.Duration
-
 	// writeMu serializes writes to conn, which come from
 	// serve(), from the relay goroutine and, for KILL, from
 	// Disconnect.
@@ -100,9 +95,6 @@ func newConn(
 		serverSfx: srvName,
 		remoteIP:  host,
 		hostname:  resolveHost(ctx, host),
-
-		pingInterval: pingInterval,
-		pongDeadline: pongDeadline,
 	}
 
 	conn.commands = conn.buildCommandMap()
@@ -221,15 +213,18 @@ func resolveHost(ctx context.Context, addr string) string {
 
 // serve is the main loop for a single IRC client connection.
 // Cancelling ctx when it returns stops the relay goroutine.
-// When nothing has been read for pingInterval the client is
-// sent a PING, and when nothing is read within pongDeadline
-// after that the connection is closed.
-func (c *Conn) serve(ctx context.Context) {
+// 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,
+	interval, deadline time.Duration,
+) {
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 	defer c.cleanup(ctx)
 
-	ping := time.AfterFunc(c.pingInterval, func() {
+	ping := time.AfterFunc(interval, func() {
 		c.send("PING :" + c.serverSfx)
 	})
 	defer ping.Stop()
@@ -239,10 +234,10 @@ func (c *Conn) serve(ctx context.Context) {
 
 	for {
 		// Every line read starts both waits again.
-		ping.Reset(c.pingInterval)
+		ping.Reset(interval)
 
 		_ = c.conn.SetReadDeadline(
-			time.Now().Add(c.pingInterval + c.pongDeadline),
+			time.Now().Add(interval + deadline),
 		)
 
 		if !scanner.Scan() {

internal/ircserver/export_test.go

@@ -13,13 +13,16 @@ import (
 	"sneak.berlin/go/neoirc/internal/service"
 )
 
-// NewTestServer creates a Server suitable for testing.
+// NewTestServer creates a Server suitable for testing, whose
+// connections use interval and deadline in place of
+// pingInterval and pongDeadline.
 // The caller must call Stop() when finished.
 func NewTestServer(
 	log *slog.Logger,
 	cfg *config.Config,
 	database *db.Database,
 	brk *broker.Broker,
+	interval, deadline time.Duration,
 ) *Server {
 	globs := &globals.Globals{
 		Appname:   "neoirc",
@@ -38,6 +41,9 @@ func NewTestServer(
 		brk:      brk,
 		svc:      svc,
 		conns:    make(map[*Conn]struct{}),
+
+		pingInterval: interval,
+		pongDeadline: deadline,
 	}
 }
 
@@ -64,22 +70,13 @@ const KillWriteWindow = killWriteWindow
 // output queue when no broker notification arrives.
 const PollInterval = pollInterval
 
-// ServeConn serves tcpConn as s serves each connection it
-// accepts, except that interval and deadline replace
-// pingInterval and pongDeadline.
-func (s *Server) ServeConn(
-	tcpConn net.Conn,
-	interval, deadline time.Duration,
-) {
-	client := newConn(
-		context.Background(), tcpConn, s.log,
-		s.database, s.brk, s.cfg, s.svc,
-	)
-	client.pingInterval = interval
-	client.pongDeadline = deadline
+// PingInterval is how long the server waits with nothing read
+// from a connection before it sends a PING.
+const PingInterval = pingInterval
 
-	go client.serve(context.Background())
-}
+// PongDeadline is how long the server waits after the PING
+// for a line before it closes the connection.
+const PongDeadline = pongDeadline
 
 // NewTestConn returns a Conn for nick on tcpConn, without a
 // server, database or service behind it.

internal/ircserver/server.go

@@ -6,6 +6,7 @@ import (
 	"log/slog"
 	"net"
 	"sync"
+	"time"
 
 	"go.uber.org/fx"
 	"sneak.berlin/go/neoirc/internal/broker"
@@ -38,6 +39,11 @@ type Server struct {
 	mu       sync.Mutex
 	conns    map[*Conn]struct{}
 	cancel   context.CancelFunc
+
+	// pingInterval and pongDeadline are the constants of the
+	// same names, except in tests, which shorten them.
+	pingInterval time.Duration
+	pongDeadline time.Duration
 }
 
 // New creates a new IRC Server and registers its lifecycle
@@ -57,6 +63,9 @@ func New(
 		listener: nil,
 		cancel:   nil,
 		mu:       sync.Mutex{},
+
+		pingInterval: pingInterval,
+		pongDeadline: pongDeadline,
 	}
 
 	listenAddr := params.Config.IRCListenAddr
@@ -153,7 +162,7 @@ func (s *Server) acceptLoop(ctx context.Context) {
 				s.mu.Unlock()
 			}()
 
-			client.serve(ctx)
+			client.serve(ctx, s.pingInterval, s.pongDeadline)
 		}()
 	}
 }

internal/ircserver/server_test.go

@@ -71,6 +71,22 @@ func newTestEnvWithConfig(
 ) *testEnv {
 	t.Helper()
 
+	return newTestEnvWithPingTimes(
+		t, cfg, ircserver.PingInterval, ircserver.PongDeadline,
+	)
+}
+
+// newTestEnvWithPingTimes is newTestEnvWithConfig, except
+// that the server sends a connection a PING after interval
+// with nothing read, and closes it when nothing is read
+// within deadline after that.
+func newTestEnvWithPingTimes(
+	t *testing.T,
+	cfg *config.Config,
+	interval, deadline time.Duration,
+) *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(
@@ -127,7 +143,9 @@ func newTestEnvWithConfig(
 		&slog.HandlerOptions{Level: slog.LevelError}, //nolint:exhaustruct
 	))
 
-	srv := ircserver.NewTestServer(log, cfg, database, brk)
+	srv := ircserver.NewTestServer(
+		log, cfg, database, brk, interval, deadline,
+	)
 
 	err = srv.Start(addr)
 	if err != nil {
@@ -175,34 +193,26 @@ func (env *testEnv) dial(t *testing.T) *testClient {
 	}
 }
 
-// The ping interval and PONG deadline for dialWithPingTimes.
-// They are equal, so that three answered PINGs take longer
-// than a client that answers none stays connected.
+// The ping interval and PONG deadline for
+// newTestEnvWithShortPings. They are equal, so that three
+// answered PINGs take longer than a client that answers none
+// stays connected.
 const (
 	testPingInterval = 300 * time.Millisecond
 	testPongDeadline = 300 * time.Millisecond
 )
 
-// dialWithPingTimes connects a client to the test server
-// whose connection is sent a PING after testPingInterval with
-// nothing read, and closed when nothing is read within
-// testPongDeadline after that.
-func (env *testEnv) dialWithPingTimes(t *testing.T) *testClient {
+// newTestEnvWithShortPings starts an IRC server that uses
+// testPingInterval and testPongDeadline in place of
+// pingInterval and pongDeadline.
+func newTestEnvWithShortPings(t *testing.T) *testEnv {
 	t.Helper()
 
-	serverSide, clientSide := net.Pipe()
-
-	t.Cleanup(func() { _ = clientSide.Close() })
-
-	env.srv.ServeConn(
-		serverSide, testPingInterval, testPongDeadline,
+	return newTestEnvWithPingTimes(
+		t,
+		&config.Config{ServerName: testServerName},
+		testPingInterval, testPongDeadline,
 	)
-
-	return &testClient{
-		t:       t,
-		conn:    clientSide,
-		scanner: bufio.NewScanner(clientSide),
-	}
 }
 
 // isServerPing reports whether line is the PING the test
@@ -432,8 +442,8 @@ func TestPingPong(t *testing.T) {
 func TestQuietClientAnsweringPingStaysConnected(t *testing.T) {
 	t.Parallel()
 
-	env := newTestEnv(t)
-	client := env.dialWithPingTimes(t)
+	env := newTestEnvWithShortPings(t)
+	client := env.dial(t)
 	client.register("answers")
 
 	for range 3 {
@@ -450,8 +460,8 @@ func TestQuietClientAnsweringPingStaysConnected(t *testing.T) {
 func TestQuietClientNotAnsweringPingIsClosed(t *testing.T) {
 	t.Parallel()
 
-	env := newTestEnv(t)
-	client := env.dialWithPingTimes(t)
+	env := newTestEnvWithShortPings(t)
+	client := env.dial(t)
 	client.register("silent")
 
 	lines := client.readUntilClosed()
@@ -466,8 +476,8 @@ func TestQuietClientNotAnsweringPingIsClosed(t *testing.T) {
 func TestUnregisteredClientAnswersPing(t *testing.T) {
 	t.Parallel()
 
-	env := newTestEnv(t)
-	client := env.dialWithPingTimes(t)
+	env := newTestEnvWithShortPings(t)
+	client := env.dial(t)
 
 	client.readUntil(isServerPing)
 	client.send("PONG :" + testServerName)