fix: harden resume/shutdown paths, tighten Intent-18 and PSK handshake handling
Client (Go) — resume correctness - C1: completeResume now re-parks the stream when replay fails mid-conn-loss. parked was cleared before the replay loop, so the dying conn's teardown would start a second resumeLoop and the two loops could strand the stream with neither alive. Resume stats are counted only after the replay lands. - C2: RST(ALREADY_BOUND) is retryable instead of terminating the loop. With C1 fixed there is never a genuine second attempt, so "already bound" means the hub still holds the stream on a half-open conn; the retry waits out that bind (bounded by the grace deadline, teardown on expiry) instead of returning and leaving the destination socket hung forever. Client (Go) — shutdown semantics - C3: Close() sets a closing flag and cancels an internal context; dialSession takes a ctx (DialContext + AfterFunc so shutdown aborts in-flight handshakes); the worker pool refuses new conns after closeAll (Allocate, background growth, cond waiters); serveControl's reconnect loop is gated by closing so Close works even when the caller's Start context is not cancelled; conn-loss teardown closes streams outright during shutdown instead of parking them for a reattach that will never come. Client (Go) — hygiene - C4: pingInterval() clamps at the single point a duration is derived, so a hand-built Config with PingIntervalMs <= 0 can no longer panic time.NewTicker (added DefaultPingIntervalMs). - E6: shaperStall is sampled right after shaper.Acquire, before the socket write, so a hub that is not reading is no longer charged to the bandwidth cap in the stats. - E7: stream log lines now carry conn%d/sid%d (leg.String()), making streams traceable across reattaches. - P5: mirror constants IntentReserved/RegisterOk/RegisterErrPattern added; RegisterAck dispatch logs rejection reasons via the named codes. Hub (Java) + PROTOCOL.md - P3: Intent 18 replies with a Minecraft status-response packet ([Len: VarInt][0x00][JSON: String]) and closes (socket.end, so the write always lands) instead of closing silently; documented in PROTOCOL.md §2. - P4: PSK address check is strict equality with the lowercase hex address; an uppercase/case-folded variant is now rejected per PROTOCOL.md §2. - P7: PROTOCOL.md §5 SessionReady row lists its real fields (Flags/RecvWindow/ResumeGraceMs) instead of "(none)". Verified: go vet, go test -race ./client/..., gradle test, full e2e suite (twice), resume e2e 3x, plus live probes of the hub with the real client codec (Intent-18 status reply, strict-lowercase PSK acceptance/rejection).%
This commit is contained in:
+56
-5
@@ -1,6 +1,8 @@
|
||||
package client
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"log"
|
||||
"net"
|
||||
"sync"
|
||||
@@ -10,6 +12,11 @@ import (
|
||||
"github.com/iceBear67/redapricot/client/wire"
|
||||
)
|
||||
|
||||
// errPoolClosed is returned by Allocate after Close: the pool is shutting down
|
||||
// and must not start new dials, so a caller (handleControlRequest,
|
||||
// allocateForResume) gives up rather than wait on a cond no one will satisfy.
|
||||
var errPoolClosed = errors.New("worker pool closed")
|
||||
|
||||
// StreamsBeforeGrowing is how many streams a worker conn may carry before the
|
||||
// pool starts opening another one. Set to 1 so the pool fans out to maxConn
|
||||
// under load *before* stacking streams: concentrating every player on a single
|
||||
@@ -37,6 +44,7 @@ type WorkerPool struct {
|
||||
dialing int // dials currently in flight (foreground + background)
|
||||
dialGen uint64
|
||||
dialErr error // most recent dial failure
|
||||
closed bool // closeAll ran; no new conns may join the pool
|
||||
}
|
||||
|
||||
func newWorkerPool(c *Client, maxConn int) *WorkerPool {
|
||||
@@ -56,6 +64,13 @@ func newWorkerPool(c *Client, maxConn int) *WorkerPool {
|
||||
func (p *WorkerPool) Allocate() (*WorkerConn, int, error) {
|
||||
p.mu.Lock()
|
||||
for {
|
||||
if p.closed {
|
||||
// Close won. No new conn may join the pool, so no stream may be
|
||||
// placed; waiting on the cond could only be satisfied by a dial we
|
||||
// must not start.
|
||||
p.mu.Unlock()
|
||||
return nil, 0, errPoolClosed
|
||||
}
|
||||
best, bestCount := p.leastLoadedLocked()
|
||||
if best != nil {
|
||||
p.maybeGrowLocked(bestCount)
|
||||
@@ -86,6 +101,15 @@ func (p *WorkerPool) Allocate() (*WorkerConn, int, error) {
|
||||
p.mu.Unlock()
|
||||
return nil, 0, err
|
||||
}
|
||||
if p.closed {
|
||||
// Close raced this dial: the conn must not enter the pool. Closing
|
||||
// it here, under p.mu, is a raw socket close — fine, and it makes
|
||||
// the shutdown atomic with the pool state.
|
||||
p.cond.Broadcast()
|
||||
p.mu.Unlock()
|
||||
_ = wc.fc.Close()
|
||||
return nil, 0, errPoolClosed
|
||||
}
|
||||
p.conns = append(p.conns, wc)
|
||||
p.cond.Broadcast()
|
||||
}
|
||||
@@ -113,6 +137,9 @@ func (p *WorkerPool) maybeGrowLocked(bestCount int) {
|
||||
if bestCount < StreamsBeforeGrowing {
|
||||
return
|
||||
}
|
||||
if p.closed {
|
||||
return // shutdown; do not start dials nobody will join the pool
|
||||
}
|
||||
if len(p.conns)+p.dialing >= p.maxConn {
|
||||
if bestCount > SaturationThreshold {
|
||||
log.Printf("worker pool at maxConn=%d with %d streams on the least-loaded conn", p.maxConn, bestCount)
|
||||
@@ -129,6 +156,8 @@ func (p *WorkerPool) maybeGrowLocked(bestCount int) {
|
||||
p.dialErr = err
|
||||
switch {
|
||||
case err != nil:
|
||||
case p.closed:
|
||||
surplus = wc // Close raced this background dial
|
||||
case len(p.conns) < p.maxConn:
|
||||
p.conns = append(p.conns, wc)
|
||||
default:
|
||||
@@ -146,8 +175,9 @@ func (p *WorkerPool) maybeGrowLocked(bestCount int) {
|
||||
}
|
||||
|
||||
// dialWorker establishes one worker conn. It must be called without p.mu held.
|
||||
// The dial runs on the client's context so Close aborts it mid-handshake.
|
||||
func (p *WorkerPool) dialWorker() (*WorkerConn, error) {
|
||||
sess, err := p.client.dialSession(MagicWorker)
|
||||
sess, err := p.client.dialSession(p.client.ctx, MagicWorker)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
@@ -200,7 +230,11 @@ func (p *WorkerPool) remove(wc *WorkerConn) {
|
||||
|
||||
func (p *WorkerPool) closeAll() {
|
||||
p.mu.Lock()
|
||||
p.closed = true
|
||||
conns := append([]*WorkerConn(nil), p.conns...)
|
||||
// Wake waiters parked in Allocate: the closed flag they re-check is the
|
||||
// only thing that can release them now that no conn will ever join.
|
||||
p.cond.Broadcast()
|
||||
p.mu.Unlock()
|
||||
for _, wc := range conns {
|
||||
_ = wc.fc.Close()
|
||||
@@ -401,6 +435,14 @@ func (wc *WorkerConn) readLoop() {
|
||||
// Only the tunnel leg died. Where the session negotiated resumption the
|
||||
// destination sockets are kept open and each stream reattaches over a fresh
|
||||
// conn (§7.5); otherwise this is the old, unconditional teardown.
|
||||
// During Close there is no reattach to come: a stream that parked now would
|
||||
// hold its destination socket open past shutdown, so teardown instead.
|
||||
if wc.pool.client.closing.Load() {
|
||||
for _, st := range streams {
|
||||
st.teardown(false)
|
||||
}
|
||||
return
|
||||
}
|
||||
for _, st := range streams {
|
||||
if !st.park(wc.grace) {
|
||||
st.teardown(false)
|
||||
@@ -445,6 +487,11 @@ type leg struct {
|
||||
sid int
|
||||
}
|
||||
|
||||
// String formats one (conn, sid) snapshot for log correlation. Stream ids
|
||||
// restart at 1 per conn, so a bare sid cannot be traced across a reattach —
|
||||
// the conn id is what ties the log lines together.
|
||||
func (lg *leg) String() string { return fmt.Sprintf("conn%d/sid%d", lg.wc.id, lg.sid) }
|
||||
|
||||
// Stream bridges one player (via the hub) to one destination connection.
|
||||
//
|
||||
// Data from the hub is queued and written to the destination by a dedicated
|
||||
@@ -556,7 +603,7 @@ func (s *Stream) run() {
|
||||
dest, err := net.DialTimeout("tcp", s.mapping.Destination, 10*time.Second)
|
||||
if err != nil {
|
||||
lg := s.conn()
|
||||
log.Printf("stream %d: dial %s failed: %v", lg.sid, s.mapping.Destination, err)
|
||||
log.Printf("stream %s: dial %s failed: %v", lg, s.mapping.Destination, err)
|
||||
lg.wc.removeStream(lg.sid)
|
||||
lg.wc.sendRst(lg.sid)
|
||||
s.teardown(false)
|
||||
@@ -569,7 +616,7 @@ func (s *Stream) run() {
|
||||
if s.mapping.ProxyProtocol {
|
||||
if hdr := s.buildProxyHeader(dest); hdr != nil {
|
||||
if _, err := dest.Write(hdr); err != nil {
|
||||
log.Printf("stream %d: proxy header write: %v", s.conn().sid, err)
|
||||
log.Printf("stream %s: proxy header write: %v", s.conn(), err)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -637,6 +684,10 @@ func (s *Stream) sendToHub(data []byte) bool {
|
||||
if !s.client.shaper.Acquire(&s.share, n, s.done) {
|
||||
return false
|
||||
}
|
||||
// Sampled here, before the socket write: emit's write to the hub can
|
||||
// block when the hub is not reading, and charging that time to the
|
||||
// bandwidth cap would blame maxBandwidth for a hub that is not draining.
|
||||
waited := shaperStall.elapsed()
|
||||
if !s.emit(data[:n]) {
|
||||
return false
|
||||
}
|
||||
@@ -645,7 +696,7 @@ func (s *Stream) sendToHub(data []byte) bool {
|
||||
// Separated from the window stall on purpose: this one says the
|
||||
// configured cap is the binding constraint, and raising maxBandwidth
|
||||
// is the fix. The window stall says the opposite.
|
||||
s.stats.shaperStall += shaperStall.elapsed()
|
||||
s.stats.shaperStall += waited
|
||||
s.stats.bytesUp += int64(n)
|
||||
s.mu.Unlock()
|
||||
}
|
||||
@@ -754,7 +805,7 @@ func (s *Stream) deliverFromHub(data []byte) {
|
||||
if s.qBytes+len(data) > s.client.streamWnd {
|
||||
s.mu.Unlock()
|
||||
lg := s.conn()
|
||||
log.Printf("stream %d: peer exceeded flow-control window; resetting", lg.sid)
|
||||
log.Printf("stream %s: peer exceeded flow-control window; resetting", lg)
|
||||
lg.wc.removeStream(lg.sid)
|
||||
lg.wc.sendRst(lg.sid)
|
||||
s.teardown(false)
|
||||
|
||||
Reference in New Issue
Block a user