Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bindings: mark Connection as Sync #4467

Merged
merged 5 commits into from
Mar 25, 2024
Merged

bindings: mark Connection as Sync #4467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
94dba4a
bindings: add Sync to Connection
lrstewart Mar 21, 2024
f1bed40
bump minor version: ConnectionFuture changed
lrstewart Mar 21, 2024
5e1093d
cargo fmt
lrstewart Mar 21, 2024
36734d0
Much more safety documentation
lrstewart Mar 25, 2024
1da6112
Revert "Much more safety documentation"
lrstewart Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bindings/rust/s2n-tls-sys/templates/Cargo.template
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
name = "s2n-tls-sys"
description = "A C99 implementation of the TLS/SSL protocols"
version = "0.1.7"
version = "0.2.0"
authors = ["AWS s2n"]
edition = "2021"
rust-version = "1.63.0"
Expand Down
4 changes: 2 additions & 2 deletions bindings/rust/s2n-tls-tokio/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
name = "s2n-tls-tokio"
description = "An implementation of TLS streams for Tokio built on top of s2n-tls"
version = "0.1.7"
version = "0.2.0"
authors = ["AWS s2n"]
edition = "2021"
rust-version = "1.63.0"
Expand All @@ -15,7 +15,7 @@ default = []
errno = { version = "0.3" }
libc = { version = "0.2" }
pin-project-lite = { version = "0.2" }
s2n-tls = { version = "=0.1.7", path = "../s2n-tls" }
s2n-tls = { version = "=0.2.0", path = "../s2n-tls" }
tokio = { version = "1", features = ["net", "time"] }

[dev-dependencies]
Expand Down
4 changes: 2 additions & 2 deletions bindings/rust/s2n-tls/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
name = "s2n-tls"
description = "A C99 implementation of the TLS/SSL protocols"
version = "0.1.7"
version = "0.2.0"
authors = ["AWS s2n"]
edition = "2021"
rust-version = "1.63.0"
Expand All @@ -19,7 +19,7 @@ testing = ["bytes"]
bytes = { version = "1", optional = true }
errno = { version = "0.3" }
libc = "0.2"
s2n-tls-sys = { version = "=0.1.7", path = "../s2n-tls-sys", features = ["internal"] }
s2n-tls-sys = { version = "=0.2.0", path = "../s2n-tls-sys", features = ["internal"] }
pin-project-lite = "0.2"
hex = "0.4"

Expand Down
2 changes: 1 addition & 1 deletion bindings/rust/s2n-tls/src/callbacks/async_cb.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use std::pin::Pin;
/// if it wants to run an asynchronous operation (disk read, network call).
/// The application can return an error ([Err(Error::application())])
/// to indicate connection failure.
pub trait ConnectionFuture: 'static + Send {
pub trait ConnectionFuture: 'static + Send + Sync {
fn poll(
self: Pin<&mut Self>,
connection: &mut Connection,
Expand Down
2 changes: 1 addition & 1 deletion bindings/rust/s2n-tls/src/callbacks/client_hello.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl<F: 'static + Send + Future<Output = Result<Config, Error>>> ConfigResolver<
}
}

impl<F: 'static + Send + Future<Output = Result<Config, Error>>> ConnectionFuture
impl<F: 'static + Send + Sync + Future<Output = Result<Config, Error>>> ConnectionFuture
for ConfigResolver<F>
{
fn poll(
Expand Down
22 changes: 22 additions & 0 deletions bindings/rust/s2n-tls/src/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,21 @@ impl fmt::Debug for Connection {
/// s2n_connection objects can be sent across threads
unsafe impl Send for Connection {}

/// # Sync
///
/// Although NonNull isn't Sync and allows access to mutable pointers even from
/// immutable references, the Connection interface enforces that all mutating
/// methods correctly require &mut self.
///
/// Developers and reviewers MUST ensure that new methods correctly use
/// either &self or &mut self depending on their behavior. No mechanism enforces this.
///
/// Note: Although non-mutating methods like getters should be thread-safe by definition,
/// technically the only thread safety guarantee provided by the underlying C library
/// is that s2n_send and s2n_recv can be called concurrently.
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this PR is a good time to take a whack at #4413

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, but you may regret that ask because the PR is much larger now :)

Please make sure all my reasoning makes sense. I'm trying to walk a fine line between explaining what Send and Sync mean (NOT what our documentation should do-- there's a lot of Rust documentation specifically on that topic) and not being helpful at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, too much larger. I don't want discussion of the documentation changes to block the Sync fix. I've moved the documentation changes to another PR: #4471

unsafe impl Sync for Connection {}

impl Connection {
pub fn new(mode: Mode) -> Self {
crate::init::init();
Expand Down Expand Up @@ -1017,4 +1032,11 @@ mod tests {
fn assert_send<T: 'static + Send>() {}
assert_send::<Context>();
}

// ensure the connection context is sync
#[test]
fn context_sync_test() {
fn assert_sync<T: 'static + Sync>() {}
assert_sync::<Context>();
}
}
Loading