Server: timeouts for the status page server

CHANGELOG.md

@@ -28,6 +28,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.

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) {

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)")

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")
+	}
+}

site/data.json

@@ -1303,6 +1303,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 ...",
@@ -1330,7 +1331,7 @@
    },
    "counts": {
     "Added": 18,
-    "Fixed": 11,
+    "Fixed": 12,
     "Changed": 10
    }
   },

site/index.html

@@ -362,6 +362,7 @@ <h4>Added</h4>
 </ul>
 <h4>Fixed</h4>
 <ul>
+<li>**Server:** the <code>-http</code> 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 ...</li>
 <li>**Server:** <code>SET_ROOM_DATA</code> no longer lets the owner make the room&#x27;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 ...</li>
 <li>**Server:** A search no longer disconnects the player when the rooms&#x27; data is large: the reply lists only as many rooms as fit in one packet, and the player pages on from there.</li>
 <li>**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 (<code>writeTimeout</code>), so its room mates&#x27; requests get their replies ...</li>