Repository navigation
connection: harden shutdown, EOF, and concurrent I/O handling - #56
Merged
Merged
Conversation
Replace the close-notify flags with an explicit connection state machine. The previous boolean and default alert value could not reliably distinguish a clean shutdown, a pending fatal alert, and a failed connection, and the shared state was unsafe when a reader and writer ran concurrently. Declare precise read and write error sets so Reader.err and Writer.err remain switchable, while ReadFailed and WriteFailed continue to expose failures from the underlying transport. Distinguish EOF at a TLS record boundary from EOF in the middle of a record. The latter is always TlsConnectionTruncated. Record-boundary EOF is accepted by default because it is common among otherwise compatible servers, while the new strict_close_notify option lets callers require a proper TLS shutdown when their protocol needs truncation protection. Make close idempotent, defer read-side fatal alerts to the writer, reject writes after terminal failure, and treat user_canceled as a warning rather than a shutdown. Support one concurrent reader and writer by making state transitions atomic and limiting TLS 1.3 key updates to their respective cipher direction. Keep incomplete input and insufficient output space retriable in the nonblocking API. Partial trailing records are left for the next decrypt call, and key-update requests are claimed only once their response is certain to fit. Update examples to expose underlying I/O errors and add tests for shutdown orderings, truncation, partial records, key updates, and reader/writer races, including ThreadSanitizer coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a big change, I'm sorry, but there is no way to split it further. My main motivations are
I spent a lot of time thinking about the
reader.errandwriter.errfields. For the transport errors, when the source stream fails, there are three main options for how to handle, keep theerrasnull(std.crypto.tls.Clientdoes this), storeReadFailedinerr(std.compressdoes this, but not consistently), or rename it and store e.g.TransportReadFailedinerr. I originally went forTransportReadFailed, to make it super obvious, but then chickened out and went back toReadFailed. I'm hoping the interface will change in the future, but if not, this is a good enough pattern. I'm also open to making itnull, to make it compatible withstd.crypto.tlsand makes the user code a bit nicer.https://ziggit.dev/t/making-it-harder-to-swallow-error-canceled/16556/58?u=lalinsky
Details from the commit message:
Replace the close-notify flags with an explicit connection state machine. The previous boolean and default alert value could not reliably distinguish a clean shutdown, a pending fatal alert, and a failed connection, and the shared state was unsafe when a reader and writer ran concurrently.
Declare precise read and write error sets so Reader.err and Writer.err remain switchable, while ReadFailed and WriteFailed continue to expose failures from the underlying transport.
Distinguish EOF at a TLS record boundary from EOF in the middle of a record. The latter is always TlsConnectionTruncated. Record-boundary EOF is accepted by default because it is common among otherwise compatible servers, while the new strict_close_notify option lets callers require a proper TLS shutdown when their protocol needs truncation protection.
Make close idempotent, defer read-side fatal alerts to the writer, reject writes after terminal failure, and treat user_canceled as a warning rather than a shutdown. Support one concurrent reader and writer by making state transitions atomic and limiting TLS 1.3 key updates to their respective cipher direction.
Keep incomplete input and insufficient output space retriable in the nonblocking API. Partial trailing records are left for the next decrypt call, and key-update requests are claimed only once their response is certain to fit.
Update examples to expose underlying I/O errors and add tests for shutdown orderings, truncation, partial records, key updates, and reader/writer races, including ThreadSanitizer coverage.