Implement the prereadable connection - #47
Conversation
There was a problem hiding this comment.
Pull request overview
Implements a new internal conn package that wraps *net.TCPConn to support a “preread” mode: reads are buffered and can be rewound/replayed so callers can sniff initial bytes (e.g., SNI/Host) without losing stream integrity.
Changes:
- Added
internal/conn.Connwith preread buffering, rewind, preread limit enforcement, and optional metrics hooks. - Added an extensive test suite covering limit behavior, replay boundaries, EOF handling, and concurrency invariants.
- Updated module Go version and dependency set.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/conn/conn.go | Adds the preread-capable TCPConn wrapper, options, and metrics hooks. |
| internal/conn/conn_test.go | Adds comprehensive unit tests for preread/replay/limit/EOF and concurrency behaviors. |
| go.mod | Updates Go version and introduces new module requirements/indirect requirements. |
| go.sum | Adds checksums for new/updated module dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if m == nil { | ||
| panic("nil metrics") | ||
| } | ||
| if v := reflect.ValueOf(m); v.Kind() == reflect.Pointer && v.IsNil() { |
| go 1.25.0 | ||
|
|
|
|
||
| const defaultPrereadLimit = 64 * 1024 | ||
|
|
||
| var ErrPrereadLimitExceeded = errors.New("exceed preread limit") |
yhaliaw
left a comment
yhaliaw
left a comment
There was a problem hiding this comment.
Major changes needed
🤝 Human review with AI assistance.
| // ReadFrom is not supported. | ||
| func (c *Conn) ReadFrom(r io.Reader) (n int64, err error) { | ||
| panic("not supported") | ||
| } |
There was a problem hiding this comment.
ReadFrom panics unconditionally here, and WriteTo does the same at lines 113-116. Because *net.TCPConn is embedded (line 26), *Conn satisfies both io.ReaderFrom and io.WriterTo, and io.Copy checks those interfaces before falling back to Read/Write.
That means io.Copy(dst, conn) and io.Copy(conn, src) panic on the very first call. The repo's existing relay, RelayTCP in aproxy.go (lines 277 and 284), is built on exactly those two io.Copy calls, and there's no recover() anywhere outside test code — an unrecovered panic in a per-connection goroutine takes down the whole daemon. This PR doesn't wire Conn into the relay path yet, so nothing breaks today, but it's a landmine for the follow-up PR.
The cleanest fix is to stop embedding *net.TCPConn and instead hold it as an unexported field, forwarding only the safe methods (Close, CloseWrite, LocalAddr, RemoteAddr, the deadline setters). That removes ReadFrom/WriteTo from the method set entirely so io.Copy falls back to buffered copying. If embedding needs to stay for another reason, at minimum return an error instead of panicking so io.Copy surfaces a normal error rather than crashing the process.
🤖 AI-assisted
Stack created with GitHub Stacks CLI • Give Feedback 💬
This is a part of a series of pull requests to create the next generation of the aproxy.
In this pull request, the core utility that enables the extraction of SNI or host name from a connection (prereadable connection) is implemented.