Skip to content

server: no WriteTimeout is configured, so a slow-reading client can park handler goroutines indefinitely #1002

Description

@Michad

The HTTP server sets ReadHeaderTimeout and nothing else, so there's no bound on how long a response write can block.

internal/server/server.go:363-368:

srv := &http.Server{
	Addr:              config.Server.BindHost + ":" + strconv.Itoa(config.Server.Port),
	BaseContext:       func(_ net.Listener) context.Context { return rootCtx },
	Handler:           rootHandler,
	ReadHeaderTimeout: time.Second,
}

No WriteTimeout, no ReadTimeout, no IdleTimeout. A client that opens a connection, sends a valid request, and then stops reading the response leaves the handler blocked in w.Write for as long as the peer keeps the socket open. Server.Timeout bounds tile generation via the request context, but it doesn't bound writing the result back.

This interacts badly with the timeout handler. internal/server/timeout_handler.go:71-77:

case <-ctx.Done():
	tw.mu.Lock()
	defer tw.mu.Unlock()
	// Nothing written by the inner handler wins after this point; timeoutWriter.Write starts rejecting writes once tw.timedOut is set.
	tw.timedOut = true
	writeError(ctx, w, h.errCfg, pkg.TimeoutError{}, config.DataTypeUnknown)

The deferred unlock doesn't run until ServeHTTP returns, so tw.mu is held for the whole of writeError, which writes the error body to the real socket. Meanwhile the inner handler goroutine is still running and blocks in timeoutWriter.Write on that same mutex the moment it tries to write. With no write deadline, both goroutines stay parked as long as the client declines to read.

The inner handler holds a generation refcount while it's parked — internal/server/tile_handler.go takes cur, release := h.acquire() with defer release(), which can't fire until the handler returns. generationRegistry.closeAll waits on waitForIdle, which polls inFlight() until the shutdown budget expires, so parked requests delay reload and graceful shutdown up to that deadline.

Setting a WriteTimeout is the containing fix and the one this issue is about. It bounds the socket write regardless of what the handler is doing, and it's a normal thing for a public-facing server to have. docs/operation/modules/ROOT/pages/productionizing.adoc already treats slow-client hardening as in scope for the hardened mode, so a default plus a config key fits the existing shape. Some care needed on the value: it has to exceed Server.Timeout plus the time to write a large tile, or slow-but-legitimate clients start failing. Making it configurable with a default derived from Server.Timeout is probably the right call.

The mutex scope in timeout_handler.go is worth narrowing independently — only the tw.timedOut = true store needs the lock, and holding it across a socket write serves no purpose — but a write deadline is what actually bounds the failure.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions