http: bound websocket writes with deadlines #4

The /logs and /events handlers wrote frames with conn.WriteMessage and never set a write deadline. A client that stopped reading but kept the TCP connection open could fill the kernel send buffer and park the handler goroutine on a write forever, leaking the request context, the broker subscription, and the log producer.

Add a wsWriteWait constant (10s) and call SetWriteDeadline before every WriteMessage in the logs drain loop and in streamEvents. The keep-alive ping and the closing close-frame already used WriteControl, which takes a deadline argument directly; raise their bounds from 1s to wsWriteWait for consistency. A stuck peer now fails the next write within ~10s and the handler unwinds cleanly.

+39 -3

1 changed file

Diff round #0

expand all

http.go

+39 -3

http.go

··· 34 34 "go.mitchellh.com/tack/internal/buildkite" 35 35 ) 36 36 37 + // wsWriteWait bounds how long any single websocket write (frame or 38 + // control) is allowed to block before we treat the peer as dead. A 39 + // client that stops reading but keeps the TCP connection open would 40 + // otherwise hang the handler indefinitely on a full kernel send buffer. 41 + // 10s is intentionally generous: real backpressure resolves in 42 + // milliseconds, so anything past that is a stuck peer we'd rather drop 43 + // than serve. 44 + const wsWriteWait = 10 * time.Second 45 + 37 46 // runHTTP starts the spindle's HTTP server and blocks until ctx is 38 47 // cancelled or the listener returns a fatal error. On ctx cancellation it 39 48 // performs a graceful shutdown with a bounded timeout. ··· 285 294 defer func() { 286 295 // Send a close frame on the way out so the appview proxy 287 296 // sees a clean shutdown. Mirrors upstream 288 - // spindle.(*Spindle).Logs. 297 + // spindle.(*Spindle).Logs. WriteControl honours the 298 + // deadline argument directly, so a stuck peer can't hang 299 + // us here. 289 300 _ = conn.WriteControl( 290 301 websocket.CloseMessage, 291 302 websocket.FormatCloseMessage( 292 303 websocket.CloseNormalClosure, "log stream complete", 293 304 ), 294 - time.Now().Add(time.Second), 305 + time.Now().Add(wsWriteWait), 295 306 ) 296 307 conn.Close() 297 308 }() ··· 334 345 ) 335 346 return 336 347 } 348 + // Bound the write so a client that stopped reading 349 + // but kept the TCP connection open can't hang us on a 350 + // full kernel send buffer. WriteMessage doesn't take a 351 + // deadline argument the way WriteControl does — we 352 + // have to set it on the conn before each frame. 353 + if err := conn.SetWriteDeadline(time.Now().Add(wsWriteWait)); err != nil { 354 + logger.Debug("logs set write deadline failed", 355 + "err", err, 356 + "knot", knot, 357 + "pipeline_rkey", pipelineRkey, 358 + "workflow", workflow, 359 + ) 360 + return 361 + } 337 362 if err := conn.WriteMessage(websocket.TextMessage, frame); err != nil { 338 363 logger.Debug("logs frame write failed", 339 364 "err", err, ··· 440 465 return 441 466 } 442 467 case <-ticker.C: 468 + // WriteControl takes its own deadline argument, so 469 + // the ping itself can't hang us — but we still want a 470 + // generous-but-bounded ceiling to match the per-frame 471 + // write timeout. 443 472 if err := conn.WriteControl( 444 473 websocket.PingMessage, nil, 445 - time.Now().Add(time.Second), 474 + time.Now().Add(wsWriteWait), 446 475 ); err != nil { 447 476 logger.Debug("events ping failed", "err", err) 448 477 return ··· 475 504 if err != nil { 476 505 return fmt.Errorf("marshal envelope: %w", err) 477 506 } 507 + // Bound the per-frame write so a client that stopped reading 508 + // (but didn't close the TCP connection) can't hang the 509 + // handler on a full kernel send buffer. WriteMessage has no 510 + // deadline argument of its own — we set it on the conn. 511 + if err := conn.SetWriteDeadline(time.Now().Add(wsWriteWait)); err != nil { 512 + return fmt.Errorf("set write deadline: %w", err) 513 + } 478 514 if err := conn.WriteMessage(websocket.TextMessage, frame); err != nil { 479 515 return fmt.Errorf("write frame: %w", err) 480 516 }

History

1 round 0 comments

mitchellh.com submitted #0 19d

1 commit

expand

20672d3c

http: bound websocket writes with deadlines

merge conflicts detected

expand

http.go:34

expand 0 comments

Configure Feed

Configure Feed

History