From dba47494b475e3430760c1880c22d8c672b2737b Mon Sep 17 00:00:00 2001 From: undecuple <125844332+undecuple@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:36:28 -0600 Subject: [PATCH] Server: timeouts for the status page server --- CHANGELOG.md | 1 + server/api.go | 21 +++++++ server/main.go | 2 +- server/status_timeouts_test.go | 106 +++++++++++++++++++++++++++++++++ site/data.json | 3 +- site/index.html | 1 + 6 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 server/status_timeouts_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a37149..a0b773b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ All notable changes to this project are documented here. The format follows - **Docs and tooling:** CI compiles the C client (`client/psnr.c`, `client/examples/minimal.c`) with MSVC on Windows and does not run it there; the build instructions in `docs/integrating.md` give the MSVC command. ### Fixed +- **Server:** the `-http` status page now closes a connection that does not finish its request header within 5 seconds, its request within 10 seconds, does not read its answer within 10 seconds, or sits idle for 30 seconds. Before, the status server had no such limits. - **Server:** `SET_ROOM_DATA` no longer lets the owner make the room's own join reply too big for one packet: a write to the external or internal blob that would is refused with error 6 and changes nothing, so a member who asks for the room again always gets an answer. Before, such a write was accepted and later joins and re-joins were refused. - **Server:** A search no longer disconnects the player when the rooms' data is large: the reply lists only as many rooms as fit in one packet, and the player pages on from there. - **Server:** A player whose connection stops reading no longer freezes the other players in its room: the server drops it after 10 seconds of a stuck write (`writeTimeout`), so its room mates' requests get their replies and the stuck player is removed from the room. diff --git a/server/api.go b/server/api.go index a710a93..40cfad7 100644 --- a/server/api.go +++ b/server/api.go @@ -208,6 +208,27 @@ func (s *Server) authorized(r *http.Request) bool { return subtle.ConstantTimeCompare([]byte(got), []byte(s.httpToken)) == 1 } +// The status page answers small, quick requests, so a connection that does not send its request, or does not read the answer, in +// these times is closed. Without them one idle or slow connection held a goroutine and a file descriptor for as long as it liked. +const ( + statusReadHeaderTimeout = 5 * time.Second + statusReadTimeout = 10 * time.Second + statusWriteTimeout = 10 * time.Second + statusIdleTimeout = 30 * time.Second +) + +// newStatusServer is the -http server: the status handler with the timeouts above. +func newStatusServer(addr string, h http.Handler) *http.Server { + return &http.Server{ + Addr: addr, + Handler: h, + ReadHeaderTimeout: statusReadHeaderTimeout, + ReadTimeout: statusReadTimeout, + WriteTimeout: statusWriteTimeout, + IdleTimeout: statusIdleTimeout, + } +} + func (s *Server) HTTP() http.Handler { mux := http.NewServeMux() mux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { diff --git a/server/main.go b/server/main.go index ff15d90..3c5f11d 100644 --- a/server/main.go +++ b/server/main.go @@ -217,7 +217,7 @@ func main() { if reachable(*httpAddr) && statusToken == "" { logger.Warn("the status page lists every online ID and has no token; set -http-token or bind -http to localhost") } - hs = &http.Server{Addr: *httpAddr, Handler: s.HTTP()} + hs = newStatusServer(*httpAddr, s.HTTP()) go func() { errc <- hs.ListenAndServe() }() } else { logger.Debug("status page off (set -http to turn it on)") diff --git a/server/status_timeouts_test.go b/server/status_timeouts_test.go new file mode 100644 index 0000000..b1f21e1 --- /dev/null +++ b/server/status_timeouts_test.go @@ -0,0 +1,106 @@ +package main + +import ( + "bufio" + "errors" + "io" + "net" + "net/http" + "os" + "strings" + "testing" + "time" +) + +func TestStatusServerHasTimeouts(t *testing.T) { + hs := newStatusServer("127.0.0.1:0", http.NotFoundHandler()) + if hs.ReadHeaderTimeout != statusReadHeaderTimeout || hs.ReadTimeout != statusReadTimeout || + hs.WriteTimeout != statusWriteTimeout || hs.IdleTimeout != statusIdleTimeout { + t.Fatalf("timeouts: header %v read %v write %v idle %v", hs.ReadHeaderTimeout, hs.ReadTimeout, hs.WriteTimeout, hs.IdleTimeout) + } + for name, d := range map[string]time.Duration{"header": hs.ReadHeaderTimeout, "read": hs.ReadTimeout, "write": hs.WriteTimeout, "idle": hs.IdleTimeout} { + if d <= 0 || d > time.Minute { + t.Errorf("%s timeout %v: want between 0 and a minute", name, d) + } + } + if hs.Addr != "127.0.0.1:0" { + t.Errorf("Addr %q", hs.Addr) + } +} + +// startStatus serves the status handler through newStatusServer with short timeouts. +func startStatus(t *testing.T) string { + t.Helper() + s := NewServer(10, false) + hs := newStatusServer("", s.HTTP()) + hs.ReadHeaderTimeout = 150 * time.Millisecond + hs.IdleTimeout = 150 * time.Millisecond + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + go hs.Serve(ln) + t.Cleanup(func() { hs.Close() }) + return ln.Addr().String() +} + +// statusClosedWithin reports whether the server closes c (EOF or a reset) before d passes; a read that only times out means it kept waiting. +func statusClosedWithin(c net.Conn, r *bufio.Reader, d time.Duration) bool { + c.SetReadDeadline(time.Now().Add(d)) + _, err := r.ReadByte() + var ne net.Error + return err != nil && !(errors.As(err, &ne) && ne.Timeout()) +} + +func TestStatusClosesSlowAndIdleConnections(t *testing.T) { + addr := startStatus(t) + + slow, err := net.Dial("tcp", addr) + if err != nil { + t.Fatal(err) + } + defer slow.Close() + slow.Write([]byte("GET /healthz HTTP/1.1\r\nHost: 127.0.0.1\r\nX-Slow: ")) + br := bufio.NewReader(slow) + slow.SetReadDeadline(time.Now().Add(2 * time.Second)) + line, _ := br.ReadString('\n') // 408 is allowed; what matters is that the connection then ends + if line != "" && !strings.Contains(line, "408") { + t.Errorf("slow header: %q, want a 408 or a close", line) + } + if !statusClosedWithin(slow, br, 2*time.Second) { + t.Error("a connection that never finished its header was not closed") + } + + idle, err := net.Dial("tcp", addr) + if err != nil { + t.Fatal(err) + } + defer idle.Close() + idle.Write([]byte("GET /api/stats HTTP/1.1\r\nHost: 127.0.0.1\r\n\r\n")) + ibr := bufio.NewReader(idle) + idle.SetReadDeadline(time.Now().Add(2 * time.Second)) + resp, err := http.ReadResponse(ibr, nil) + if err != nil || resp.StatusCode != 200 { + t.Fatalf("a normal request: %v, %v", resp, err) + } + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + if !statusClosedWithin(idle, ibr, 2*time.Second) { + t.Error("an idle connection after an answer was not closed") + } +} + +// The -http server must be built by newStatusServer; a bare http.Server in main has no timeouts. +func TestMainBuildsStatusServerWithTimeouts(t *testing.T) { + src, err := os.ReadFile("main.go") + if err != nil { + t.Fatal(err) + } + text := string(src) + if !strings.Contains(text, "newStatusServer(*httpAddr, s.HTTP())") { + t.Error("main.go does not build the status server with newStatusServer") + } + if strings.Contains(text, "&http.Server{") { + t.Error("main.go builds an http.Server directly") + } +} diff --git a/site/data.json b/site/data.json index 2559d60..e6ee701 100644 --- a/site/data.json +++ b/site/data.json @@ -1302,6 +1302,7 @@ "**Docs and tooling:** CI compiles the C client (`client/psnr.c`, `client/examples/minimal.c`) with MSVC on Windows and does not run it there; the build instructions in `docs/integrating.md` give the MSVC command." ], "Fixed": [ + "**Server:** the `-http` status page now closes a connection that does not finish its request header within 5 seconds, its request within 10 seconds, does not read its answer within 10 seconds, or sits idle for 30 ...", "**Server:** `SET_ROOM_DATA` no longer lets the owner make the room's own join reply too big for one packet: a write to the external or internal blob that would is refused with error 6 and changes nothing, so a member ...", "**Server:** A search no longer disconnects the player when the rooms' data is large: the reply lists only as many rooms as fit in one packet, and the player pages on from there.", "**Server:** A player whose connection stops reading no longer freezes the other players in its room: the server drops it after 10 seconds of a stuck write (`writeTimeout`), so its room mates' requests get their replies ...", @@ -1329,7 +1330,7 @@ }, "counts": { "Added": 17, - "Fixed": 11, + "Fixed": 12, "Changed": 10 } }, diff --git a/site/index.html b/site/index.html index 3cd066a..4295c88 100644 --- a/site/index.html +++ b/site/index.html @@ -360,6 +360,7 @@

Added

Fixed