Repository navigation
fix(sm): an OnCER or OnDWR hook that closes the connection aborts processing - #74
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…cessing Settings.OnCER promised that closing the connection from the hook aborts the handshake, but the CER handler went on regardless: it published the peer's metadata, admitted the peer, called OnCEA, wrote the CEA to the closed connection and logged the failed write at Error. OnDWR had the same gap for the DWA. - diam.Conn gains Closed, reporting that Close has been called or that the server has finished with the connection. Close publishes it before closing the transport; when the read loop ends, the server first waits for the handlers already running, so a CER or DWR that arrived before the peer's FIN is still answered (RFC 6733 §5.6). Every server-side close, including Shutdown's forced close and the multistream error handler, goes through the same path. Closing Connection() directly is not observed; the hook docs say to call c.Close(). - After OnCER or OnDWR returns, the state machine stops if the connection is closed: no metadata, no admission, no CEA or DWA, no OnCEA, OnHandshake or OnDWA, and nothing logged above Debug. Implementations of diam.Conn outside this module must add Closed. Tests cover TCP and SCTP in every handler wiring and dispatch mode, a CER or DWR followed by the peer's FIN in sequential and concurrent dispatch, a hook still running when Shutdown's deadline expires, and the multistream read error. Fixes #70
50ea84b to
9a6c0ec
Compare
|
The lint failure is not from this change: staticcheck 2026.2.1 cannot read the export data of Go 1.27.2, the stable release CI now installs, so it stopped before analysing any package. Under Go 1.27.2, a staticcheck built from go-tools master (f1838cc3, which carries the fix) and golangci-lint both report no issues on this branch. The workflow pin is updated in a separate change. |
Settings.OnCERpromises that closing the connection from the hook aborts the handshake. The CER handler ignored that and carried on:OnCEA;OnDWRhad the same gap for the DWA.Change
diam.Conn.Closed()(new). It reports thatClosehas been called, or that the server has finished with the connection.Closepublishes it before closing the transport.Shutdown's forced close and the multistream error handler.Connection()directly is not observed, and the hook docs say to callc.Close().OnCERorOnDWRreturns, the state machine stops if the connection is closed. Then there are no metadata, no admission, no CEA or DWA, noOnCEA,OnHandshakeorOnDWA, and nothing logged above Debug.API change: implementations of
diam.Connoutside this module must addClosed() bool. An optional interface found throughConnAswas considered and rejected: opaque wrappers that embed only the core methods would hide it, and the abort would then fail silently.Tests
Shutdown's deadline expires produces no answer and no Error log.Closedbefore the transport closes.Shutdownor the multistream handler bypassing the flag.Validation
go build ./...,go test ./... -count=1,go test -race ./diam/... -count=1,go vet ./...,staticcheck ./...andgolangci-lint run ./...all pass, andgofmt -l .is empty. Theexamples/middlewarebuild, vet and test pass.Fixes #70
Summary by cubic
Fixes #70. A
Settings.OnCERorSettings.OnDWRhook that closes the connection now aborts processing: previously it continued publishing peer metadata, admitting the peer, callingOnCEA/OnDWA, writing the answer to the closed connection, and logging the failed write at Error.Closed()todiam.Conn;Close()publishes it before closing the transport, and read-loop exit waits for running handlers first so a CER or DWR received before the peer's FIN is still answered (RFC 6733 §5.6).Shutdown's forced close and the multistream error handler, now publishClosed. ClosingConnection()directly is not detected; the hook docs say to callc.Close().Breaking change
diam.Connimplementations must addClosed() bool.Written for commit 9a6c0ec. Summary will update on new commits.