Skip to content

Fix client read loop spinning and stream message reading - #289

Open
GerardGao wants to merge 2 commits into
pion:mainfrom
GerardGao:fix/client-read-loop-spin
Open

Fix client read loop spinning and stream message reading#289
GerardGao wants to merge 2 commits into
pion:mainfrom
GerardGao:fix/client-read-loop-spin

Conversation

@GerardGao

@GerardGao GerardGao commented Aug 27, 2026

Copy link
Copy Markdown

This PR fixes two issues in the client read loop.

1. Exit read loop on permanent connection error

Client.readUntilClosed ignores read errors, so once the peer closes the connection the loop spins at ~200M Read calls/sec (reproduced: 25M calls in 100ms), pinning a CPU until the client is closed.

The connection is unusable after a read error anyway: the peer may have closed it, or a stream transport may be out of sync. Exiting the loop lets the existing timeout collector terminate in-flight transactions, so the client stays consistent with RFC 5389 RTO semantics.

2. Read large and fragmented messages on streams

The client read a single 1024-byte buffer per message, which:

  • truncated messages larger than the buffer, and
  • desynchronized stream connections (TCP/TLS) whenever the server fragmented or coalesced messages.

Datagram connections now use a buffer sized to the largest possible STUN message (20-byte header + 65535-byte body). Stream connections are detected via LocalAddr and messages are reassembled from the 16-bit length field before decoding, handling fragmentation, coalescing and a final message delivered together with EOF.

Tests

Added regression tests covering both fixes:

  • read loop exits on EOF (with and without a logger), transactions still time out afterward
  • large messages on TCP and datagram connections
  • fragmented messages on TCP
  • coalesced messages on TCP
  • final message delivered together with EOF
  • malformed stream data stops the loop
  • strict mode rejects reserved message type 0

All modified lines are covered by tests; the full suite, -race, golangci-lint and the wasm build pass.

The client read loop ignored all read errors, so a closed
connection caused it to busy-spin at 100% CPU calling Read
millions of times per second until Close() was called.

Exit the loop on read error instead; in-flight transactions
are terminated by the existing timeout collector, so the
client does not go silent.
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.61%. Comparing base (d832df6) to head (69d6ac0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #289      +/-   ##
==========================================
+ Coverage   66.43%   66.61%   +0.18%     
==========================================
  Files          27       27              
  Lines        2124     2127       +3     
==========================================
+ Hits         1411     1417       +6     
+ Misses        700      698       -2     
+ Partials       13       12       -1     
Flag Coverage Δ
go 66.61% <100.00%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The client read a single buffer of 1024 bytes per message, which
truncated messages larger than the buffer and desynchronized
stream connections when messages were fragmented or coalesced.

Detect stream connections via LocalAddr and reassemble complete
messages from the length field before decoding; datagram
connections now use a buffer that fits the largest possible
STUN message.
@GerardGao GerardGao changed the title Exit read loop on permanent connection error Fix client read loop spinning and stream message reading Aug 28, 2026
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