Add CI workflow - #23
Merged
Merged
Conversation
brentalanmiller
pushed a commit
to brentalanmiller/tonic
that referenced
this pull request
Oct 6, 2023
After this commit, this crate will support using TLS streams in a half-closed state. Note that the TLS 1.3 spec in RFC 8446 says this should be supported: ``` Each party MUST send a "close_notify" alert before closing its write side of the connection, unless it has already sent some error alert. This does not have any effect on its read side of the connection. Note that this is a change from versions of TLS prior to TLS 1.3 in which implementations were required to react to a "close_notify" by discarding pending writes and sending an immediate "close_notify" alert of their own. That previous requirement could cause truncation in the read side. Both parties need not wait to receive a "close_notify" alert before closing their read side of the connection, though doing so would introduce the possibility of truncation. ``` https://tools.ietf.org/html/rfc8446#page-87 The `rustls` crate raises such a clean closure of a [`ClientSession`](https://docs.rs/rustls/0.18.0/rustls/struct.ClientSession.html#impl-Read) or [`ServerSesson`](https://docs.rs/rustls/0.18.0/rustls/struct.ServerSession.html#impl-Read) read-side with `ErrorKind::ConnectionAborted`. This crate's `TlsState` struct already encodes support for the half-closed states `TlsState::ReadShutdown` and `TlsState::WriteShutdown`, in addition to `TlsState::FullyShutdown`. However, the current behavior of the `AsyncRead` implementation is that it unconditionally shuts-down the write-half of a connection after the read-half closes cleanly with `ErrorKind::ConnectionAborted`. This change removes the `stream.session.send_close_notify()` and `this.state.shutdown_write()` calls from `poll_read()`. Note that `stream.session.send_close_notify()` is still called in `poll_shutdown()`, which the application calls to cleanly shutdown the write-half. I highly suspect the logic of this can be simplified and cleaned up further. Minimally, the edited match statement now has two identical branches which could be combined into one. Additionally, perhaps the `Stream` implementation should simply return `Ok(0)` for this case in its implementation of [`tokio::io::AsyncRead`](https://docs.rs/tokio/0.2/tokio/io/trait.AsyncRead.html), since that's the defined way to indicate clean closure with EOF from `AsyncRead`. However, I want to make the minimal changes and have them reviewed for logical correctness first. Co-authored-by: Braden Ehrat <braden@cloudflare.com>
brentalanmiller
pushed a commit
to brentalanmiller/tonic
that referenced
this pull request
Oct 6, 2023
* Support half-closed states grpc#23 * Update examples
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
1 participant
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.
No description provided.