Skip to content

fix(dmsgclient): keep-alive stream pool for the net/http-free HTTP path#3515

Merged
0pcom merged 2 commits into
skycoin:developfrom
0pcom:fix/dmsgclient-keepalive-stream-pool
Jul 19, 2026
Merged

fix(dmsgclient): keep-alive stream pool for the net/http-free HTTP path#3515
0pcom merged 2 commits into
skycoin:developfrom
0pcom:fix/dmsgclient-keepalive-stream-pool

Conversation

@0pcom

@0pcom 0pcom commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Problem

dmsg-discovery is sitting at ~84% CPU with GC burning ~2.6 cores. Production profiles (taken over dmsgpty against the live deployment):

CPU profile — the work is almost entirely garbage collection, not application logic:

function flat cum
runtime.scanObject 3.49% 57.41%
runtime.spanClass.sizeclass 24.05% 24.05%
runtime.findObject 15.00% 15.60%
runtime.sweepone 14.04%
secp256k1 Field.Sqr/Mul (real work) ~7%

Allocation profile — ~1 TB allocated over 21h uptime (~13 MB/s), and 73% of ALL allocation is under ClientSession.handshakeResponder:

dmsg.(*Client).dialSession.func2                 745 GB  73.3%
└─ dmsg.(*ClientSession).handshakeResponder      743 GB  73.1%
   ├─ noise.MakeHandshakeMessage                 168 GB  16.5%
   ├─ noise.ProcessHandshakeMessage              159 GB  15.7%
   ├─ noise.Secp256k1.DH → cipher.ECDH           144 GB  14.2%  → big.nat.make 163 GB
   ├─ dmsg.decodeGob → gob.Decode                 98 GB   9.7%
   └─ mlkem.NewEncapsulationKey768 (PQ)           55 GB   5.4%

Only 100 goroutines — no leak. It is pure per-handshake churn.

Cause: the net/http-free HTTP client sent Connection: close on every request and dialed a brand-new dmsg stream per call. Every periodic call — TPD registration (2× per 90s per visor), discovery entry re-POST, dial-path lookups — forced the server through a full noise + PQ handshake. The PQ hybrid handshake (ML-KEM768) made each one substantially more expensive, amplifying a pre-existing per-request-handshake design.

The net/http side was fixed exactly this way in dmsghttp.HTTPTransport (#3097, which also raised the server's IdleTimeout to 115s so a client-side pool can span refreshes). The net/http-free twin never got the same treatment — so it kept forcing teardowns even against the fixed server.

Fix

Send Connection: keep-alive and pool idle streams per destination.

  • streampool.go — per-dmsg.Client, per-destination idle pool. The bufio.Reader travels with the stream (a kept-alive stream may hold bytes already pulled off the wire, so pairing it with a fresh reader would silently drop them). 90s idle cap, safely below the server's 115s. ReleasePools() for client teardown.
  • Reuse only when safe — the response must be explicitly framed (Content-Length or chunked, so we stop on a clean message boundary) and the peer must not have asked to close. A read-to-EOF body spends the stream by definition and is never pooled.
  • Retry-once semantics — a failure on a reused stream retries once on a freshly-dialed one, and the retry never draws from the pool. This is what makes keep-alive safe against peers that close early.
  • discdmsg.do (the dmsg-discovery client itself) routes through the same pool — it's the most frequently polled endpoint.

Reverse compatibility

Deliberately safe in both directions:

  • Connection: keep-alive is plain HTTP/1.1; no wire-format change.
  • A server that closes anyway (including pre-v1.3.80 peers whose idle timeout is shorter than our pool's) just costs one retry — correctness is unaffected.
  • Stays net/http-free, so the TinyGo / js-wasm build is intact.

Testing

  • go build ./... clean; GOOS=js GOARCH=wasm go build clean (TinyGo path preserved).
  • go test -race ./pkg/dmsg/dmsgclient/... ./pkg/dmsg/dmsghttp/... green; ./pkg/transport/tpdclient/... green.
  • New tests: TestHTTPRoundTripKeepAliveReuse (two sequential requests over one connection — the property the pool depends on) and TestHTTPRoundTripNotReusableOnClose (a Connection: close response is never pooled).

0pcom added 2 commits July 18, 2026 19:33
dmsg-discovery sat at ~84% CPU with GC burning ~2.6 cores. A production
CPU profile showed the work was almost entirely garbage collection
(runtime.scanObject 57% cum) rather than application logic, and the
allocation profile pinned the source: 73% of ALL allocation was under
ClientSession.handshakeResponder — secp256k1 ECDH, ML-KEM (PQ), gob
decoder engines, and per-stream buffers. Only 100 goroutines, so no
leak: it was pure per-handshake churn (~1 TB allocated over 21h).

Cause: the net/http-free HTTP client sent `Connection: close` on every
request and dialed a brand-new dmsg stream per call. So every periodic
call — TPD registration (2x/90s per visor), discovery entry re-POST,
dial-path lookups — forced the server through a full noise + PQ
handshake. The net/http side was fixed exactly this way in
dmsghttp.HTTPTransport (skycoin#3097, which also raised the server's
IdleTimeout to 115s so a client pool can span refreshes); the
net/http-free twin never got the same treatment.

Send `Connection: keep-alive` and pool idle streams per destination:

- streampool.go: per-dmsg.Client, per-destination idle pool. The
  bufio.Reader travels WITH the stream (a kept-alive stream may hold
  bytes already pulled off the wire). 90s idle cap, below the server's
  115s. ReleasePools() for client teardown.
- Reuse only when the response was explicitly framed (Content-Length or
  chunked) and the peer didn't ask to close — a read-to-EOF body spends
  the stream by definition.
- A failure on a REUSED stream retries once on a freshly-dialed one;
  the retry never draws from the pool. This is what keeps keep-alive
  safe against peers that close early, including pre-v1.3.80 servers
  whose idle timeout is shorter than our pool's.
- discdmsg.do (the dmsg-discovery client itself) routes through the
  same pool — it is the most frequently polled endpoint.

Reverse-compatible in both directions: keep-alive is plain HTTP/1.1, a
server that closes anyway just costs one retry, and the wire format is
unchanged. Stays net/http-free, so the TinyGo/js-wasm build is intact
(verified with GOOS=js GOARCH=wasm).
…alive tests

The repo runs errcheck with check-blank, so `_, _ =` on io.WriteString
is not enough — annotate. And echoing r.URL.Path back into the response
tripped gosec G705 (XSS taint); use the server-side request counter
instead, which also makes the assertion prove both requests reached the
server in order over the one connection.
@0pcom
0pcom force-pushed the fix/dmsgclient-keepalive-stream-pool branch from 54eeeb8 to c3462d0 Compare July 19, 2026 00:34
@0pcom
0pcom merged commit 6187b86 into skycoin:develop Jul 19, 2026
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant