refactor: use buffered connections for I/O

[mccutchen]

Force the use of buffered I/O via bufio.ReadWriter to hopefully make reads/writes more efficient. Extracted from #23, which may or may not pan out.

[github-actions]

🔥 Run benchmarks comparing 9874501 against main:

gh workflow run bench.yaml -f pr_number=32

Note: this comment will update with each new commit.

[github-actions]

benchstats: 4e63a4d...64372fc

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                          1.026µ ± 2%   1.028µ ±  3%        ~ (p=0.159 n=10)
ReadFrame/1MiB-4                          912.0µ ± 1%   915.9µ ±  3%        ~ (p=0.165 n=10)
ReadFrame/8MiB-4                          6.983m ± 0%   7.045m ±  1%   +0.89% (p=0.002 n=10)
ReadFrame/16MiB-4                         14.17m ± 1%   14.16m ±  1%        ~ (p=0.853 n=10)
ReadMessage/1MiB/1-4                      1.071m ± 2%   1.087m ±  1%        ~ (p=0.105 n=10)
ReadMessage/8MiB/1-4                      7.764m ± 1%   7.740m ±  1%        ~ (p=0.481 n=10)
ReadMessage/16MiB/1-4                     15.66m ± 1%   15.48m ±  1%   -1.17% (p=0.000 n=10)
ReadMessage/1MiB/4-4                      1.436m ± 3%   1.500m ±  5%   +4.41% (p=0.009 n=10)
ReadMessage/8MiB/4-4                      10.55m ± 8%   10.67m ±  5%        ~ (p=1.000 n=10)
ReadMessage/16MiB/4-4                     19.39m ± 4%   19.32m ±  2%        ~ (p=0.247 n=10)
ReadMessage/1MiB/16-4                     2.258m ± 2%   2.039m ± 17%   -9.69% (p=0.023 n=10)
ReadMessage/8MiB/16-4                     15.28m ± 2%   18.29m ±  4%  +19.75% (p=0.000 n=10)
ReadMessage/16MiB/16-4                    22.44m ± 4%   23.71m ±  6%   +5.69% (p=0.001 n=10)
geomean                                   3.128m        3.176m         +1.55%

                       │ ./baseline/bench-results.txt │       ./head/bench-results.txt        │
                       │             B/op             │     B/op      vs base                 │
ReadFrame/1KiB-4                         1.055Ki ± 0%   1.055Ki ± 0%       ~ (p=1.000 n=10) ¹
ReadFrame/1MiB-4                         1.000Mi ± 0%   1.000Mi ± 0%       ~ (p=0.459 n=10)
ReadFrame/8MiB-4                         8.000Mi ± 0%   8.000Mi ± 0%       ~ (p=0.164 n=10)
ReadFrame/16MiB-4                        16.00Mi ± 0%   16.00Mi ± 0%       ~ (p=0.220 n=10)
ReadMessage/1MiB/1-4                     1.000Mi ± 0%   1.000Mi ± 0%  -0.00% (p=0.006 n=10)
ReadMessage/8MiB/1-4                     8.000Mi ± 0%   8.000Mi ± 0%       ~ (p=0.421 n=10)
ReadMessage/16MiB/1-4                    16.00Mi ± 0%   16.00Mi ± 0%       ~ (p=0.376 n=10)
ReadMessage/1MiB/4-4                     3.602Mi ± 0%   3.602Mi ± 0%  -0.00% (p=0.000 n=10)
ReadMessage/8MiB/4-4                     28.57Mi ± 0%   28.57Mi ± 0%       ~ (p=0.156 n=10)
ReadMessage/16MiB/4-4                    57.09Mi ± 0%   57.09Mi ± 0%       ~ (p=0.285 n=10)
ReadMessage/1MiB/16-4                    5.407Mi ± 0%   5.407Mi ± 0%       ~ (p=0.465 n=10)
ReadMessage/8MiB/16-4                    47.22Mi ± 0%   47.22Mi ± 0%  +0.00% (p=0.040 n=10)
ReadMessage/16MiB/16-4                   93.81Mi ± 0%   93.81Mi ± 0%  -0.00% (p=0.005 n=10)
geomean                                  5.263Mi        5.263Mi       -0.00%
¹ all samples are equal

                       │ ./baseline/bench-results.txt │      ./head/bench-results.txt       │
                       │          allocs/op           │ allocs/op   vs base                 │
ReadFrame/1KiB-4                           5.000 ± 0%   5.000 ± 0%       ~ (p=1.000 n=10) ¹
ReadFrame/1MiB-4                           5.000 ± 0%   5.000 ± 0%       ~ (p=1.000 n=10) ¹
ReadFrame/8MiB-4                           5.000 ± 0%   5.000 ± 0%       ~ (p=1.000 n=10) ¹
ReadFrame/16MiB-4                          5.000 ± 0%   5.000 ± 0%       ~ (p=1.000 n=10) ¹
ReadMessage/1MiB/1-4                       6.000 ± 0%   6.000 ± 0%       ~ (p=1.000 n=10) ¹
ReadMessage/8MiB/1-4                       6.000 ± 0%   6.000 ± 0%       ~ (p=1.000 n=10) ¹
ReadMessage/16MiB/1-4                      6.000 ± 0%   6.000 ± 0%       ~ (p=1.000 n=10) ¹
ReadMessage/1MiB/4-4                       24.00 ± 0%   24.00 ± 0%       ~ (p=1.000 n=10) ¹
ReadMessage/8MiB/4-4                       24.00 ± 0%   24.00 ± 0%       ~ (p=1.000 n=10) ¹
ReadMessage/16MiB/4-4                      24.00 ± 0%   24.00 ± 0%       ~ (p=1.000 n=10) ¹
ReadMessage/1MiB/16-4                      90.00 ± 0%   90.00 ± 0%       ~ (p=1.000 n=10) ¹
ReadMessage/8MiB/16-4                      90.00 ± 0%   90.00 ± 0%       ~ (p=1.000 n=10) ¹
ReadMessage/16MiB/16-4                     90.00 ± 0%   90.00 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                    14.59        14.59       +0.00%
¹ all samples are equal

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 79.23%. Comparing base (4e63a4d) to head (9e19bb5).

Files with missing lines	Patch %	Lines
websocket.go	50.00%	6 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
- Coverage   79.62%   79.23%   -0.40%     
==========================================
  Files           3        3              
  Lines         432      443      +11     
==========================================
+ Hits          344      351       +7     
- Misses         64       66       +2     
- Partials       24       26       +2     

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

[mccutchen]

Gonna give up on this path for now, may revisit in the future. For posterity, I'm not sure how best to reason about directing all reads and writes through a buffered wrapper and how that impacts connection state and error handling.

As a concrete example, I don't know how to make a test like this — which attempts to exercise one aspect of handling write errors, and which works well with a "plain" net.Conn — when dealing with a combination of a buffered writer and an underlying connection:

websocket/websocket_test.go

Lines 656 to 703 in 9874501

	t.Run("a write error should cause the server to close the connection", func(t *testing.T) {
	// the error we will inject into the wrapped writer, which will be
	// verified in the close frame
	writeErr := errors.New("simulated write failure")
	conn := setupRawConnWithHandler(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	// Manually re-implement websocket.Accept in order to wrap the
	// actual conn with one that will let us inject errors
	clientKey, err := websocket.Handshake(w, r)
	assert.NilError(t, err)
	hj := w.(http.Hijacker)
	realConn, _, err := hj.Hijack()
	assert.NilError(t, err)
	// Our wrapped conn will fail on the first write and succeed on
	// any subsequent writes, so echoing the incoming frame will fail
	// but writing the subsequent close frame will succeed.
	//
	// Note: I'm not sure it actually makes sense to try writing the
	// close frame after an initial write error, but that's what we
	// do for now!
	var writeCount atomic.Int64
	fakeConn := &wrappedConn{
	conn: realConn,
	write: func(b []byte) (int, error) {
	count := writeCount.Add(1)
	// return an error on first write
	if count == 1 {
	return 0, writeErr
	}
	// otherwise pass thru to underlying conn
	return realConn.Write(b)
	},
	}
	ws := websocket.New(fakeConn, clientKey, websocket.ServerMode, websocket.Options{})
	ws.Serve(r.Context(), websocket.EchoHandler)
	}))
	// We write a frame, but the server's attempt to write the echo
	// response should fail via fakeConn above.
	//
	// The _second_ write, writing the close frame before closing the
	// underlying conn, should succceed.
	mustWriteFrame(t, conn, true, websocket.NewFrame(websocket.OpcodeText, true, []byte("hello")))
	mustReadCloseFrame(t, conn, websocket.StatusServerError, writeErr)
	})