Skip to content

fix: correctly handle buffered data from hijacked conns#76

Merged
mccutchen merged 1 commit intomainfrom
fix-conn-buffering
Nov 6, 2025
Merged

fix: correctly handle buffered data from hijacked conns#76
mccutchen merged 1 commit intomainfrom
fix-conn-buffering

Conversation

@mccutchen
Copy link
Owner

I think I've finally realized what's causing the flaky test failures described in #72. As noted in a follow-up comment

if I add code like 06e053e to wrap the client and server connections and record the bytes each reads and writes, it looks like the server sometimes fails to read the first byte from a given write by the client.

This smelled a bit like buffering somewhere, and it turns out that we were intentionally (and misguidedly) discarding the buffered reader/writer returned by Hijack when capturing the connection underlying an HTTP request:

conn, _, err := hj.Hijack()

And it turns out that the Hijack docs explicitly warn us that we may be discarding buffered reads (e.g., maybe the first byte written by a client):

// The returned bufio.Reader may contain unprocessed buffered
// data from the client.

So! Here we update our Websocket to read through that buffer for all reads, while still writing directly to the underlying net.Conn. This should hopefully fix #72.

(Plus, a few misc test updates.)

@github-actions
Copy link

github-actions bot commented Nov 6, 2025

🔥 Run benchmarks comparing 62c196b against main:

gh workflow run bench.yaml -f pr_number=76

Note: this comment will update with each new commit.

@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.07%. Comparing base (351f4c6) to head (62c196b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
+ Coverage   99.04%   99.07%   +0.02%     
==========================================
  Files           2        2              
  Lines         421      431      +10     
==========================================
+ Hits          417      427      +10     
  Misses          2        2              
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 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.

@github-actions
Copy link

github-actions bot commented Nov 6, 2025

benchstats: 351f4c6...62c196b

View full benchmark output on the workflow summary.

goos: linux
goarch: amd64
pkg: github.com/mccutchen/websocket
cpu: AMD EPYC 7763 64-Core Processor                
                  │ ./baseline/bench-results.txt │      ./head/bench-results.txt      │
                  │            sec/op            │   sec/op     vs base               │
ReadFrame/1KiB-4                     698.2n ± 2%   693.4n ± 5%       ~ (p=0.853 n=10)
ReadFrame/1MiB-4                     277.7µ ± 2%   275.3µ ± 5%       ~ (p=0.684 n=10)
WriteFrame/1KiB-4                    767.7n ± 1%   781.2n ± 1%  +1.77% (p=0.004 n=10)
WriteFrame/1MiB-4                    321.2µ ± 3%   319.6µ ± 1%       ~ (p=0.353 n=10)
geomean                              14.79µ        14.78µ       -0.07%

                  │ ./baseline/bench-results.txt │      ./head/bench-results.txt       │
                  │             B/s              │     B/s       vs base               │
ReadFrame/1KiB-4                    1.378Gi ± 2%   1.387Gi ± 4%       ~ (p=0.853 n=10)
ReadFrame/1MiB-4                    3.517Gi ± 2%   3.547Gi ± 5%       ~ (p=0.684 n=10)
WriteFrame/1KiB-4                   1.253Gi ± 1%   1.231Gi ± 1%  -1.73% (p=0.004 n=10)
WriteFrame/1MiB-4                   3.041Gi ± 3%   3.056Gi ± 1%       ~ (p=0.353 n=10)
geomean                             2.073Gi        2.074Gi       +0.07%

                  │ ./baseline/bench-results.txt │       ./head/bench-results.txt        │
                  │             B/op             │     B/op      vs base                 │
ReadFrame/1KiB-4                    1.188Ki ± 0%   1.188Ki ± 0%       ~ (p=1.000 n=10) ¹
ReadFrame/1MiB-4                    1.008Mi ± 0%   1.008Mi ± 0%       ~ (p=0.714 n=10)
WriteFrame/1KiB-4                   1.148Ki ± 0%   1.148Ki ± 0%       ~ (p=1.000 n=10) ¹
WriteFrame/1MiB-4                   1.008Mi ± 0%   1.008Mi ± 0%       ~ (p=0.350 n=10)
geomean                             34.72Ki        34.72Ki       +0.00%
¹ all samples are equal

                  │ ./baseline/bench-results.txt │      ./head/bench-results.txt       │
                  │          allocs/op           │ allocs/op   vs base                 │
ReadFrame/1KiB-4                      8.000 ± 0%   8.000 ± 0%       ~ (p=1.000 n=10) ¹
ReadFrame/1MiB-4                      8.000 ± 0%   8.000 ± 0%       ~ (p=1.000 n=10) ¹
WriteFrame/1KiB-4                     4.000 ± 0%   4.000 ± 0%       ~ (p=1.000 n=10) ¹
WriteFrame/1MiB-4                     4.000 ± 0%   4.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                               5.657        5.657       +0.00%
¹ all samples are equal

@mccutchen mccutchen merged commit 0a52171 into main Nov 6, 2025
13 checks passed
@mccutchen mccutchen deleted the fix-conn-buffering branch November 6, 2025 11:24
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.

bug: flaky test suite under go 1.25

1 participant