diff --git a/bindings/rust/extended/s2n-tls-tokio/tests/handshake.rs b/bindings/rust/extended/s2n-tls-tokio/tests/handshake.rs index b550a3e53f7..ea5a74f3b66 100644 --- a/bindings/rust/extended/s2n-tls-tokio/tests/handshake.rs +++ b/bindings/rust/extended/s2n-tls-tokio/tests/handshake.rs @@ -2,12 +2,13 @@ // SPDX-License-Identifier: Apache-2.0 use rand::Rng; +#[allow(deprecated)] +use s2n_tls::pool::ConfigPoolBuilder; use s2n_tls::{ config::Config, connection::{Connection, ModifiedBuilder}, enums::{ClientAuthType, Mode, Version}, error::{Error, ErrorType}, - pool::ConfigPoolBuilder, security::{DEFAULT_TLS13, TESTING_TLS12}, }; use s2n_tls_tokio::{TlsAcceptor, TlsConnector}; @@ -45,6 +46,7 @@ async fn handshake_basic() -> Result<(), Box> { } #[tokio::test(flavor = "multi_thread")] +#[allow(deprecated)] async fn handshake_with_pool_multithread() -> Result<(), Box> { const COUNT: usize = 20; const CLIENT_LIMIT: usize = 3; @@ -109,6 +111,7 @@ async fn handshake_with_connection_config() -> Result<(), Box Result<(), Box> { fn with_client_auth(conn: &mut Connection) -> Result<&mut Connection, Error> { conn.set_client_auth_type(ClientAuthType::Optional) diff --git a/bindings/rust/extended/s2n-tls/src/connection.rs b/bindings/rust/extended/s2n-tls/src/connection.rs index e477b4d7bde..df2055899de 100644 --- a/bindings/rust/extended/s2n-tls/src/connection.rs +++ b/bindings/rust/extended/s2n-tls/src/connection.rs @@ -558,6 +558,7 @@ impl Connection { Ok(self) } + #[cfg(feature = "unstable-renegotiate")] pub(crate) fn wipe_method(&mut self, wipe: F) -> Result<(), Error> where F: FnOnce(&mut Self) -> Result, @@ -575,19 +576,31 @@ impl Connection { Ok(()) } - /// wipes an existing connection and allows it to be reused. + /// Resets a connection so that it can be reused. /// - /// This method erases all data associated with a connection including pending reads. - /// This function should be called after all I/O is completed and s2n_shutdown has been - /// called. Reusing the same connection handle(s) is more performant than repeatedly - /// calling s2n_connection_new and s2n_connection_free + /// This method no longer wipes the existing connection. Instead, it replaces the + /// connection with a newly allocated one, preserving the mode and config. /// - /// Corresponds to [`s2n_connection_wipe`]. + /// This method should be called after all I/O is completed and `Connection::poll_shutdown` + /// has been called. + #[deprecated( + note = "use `Connection::new()` instead; connection reuse provides negligible performance benefit" + )] pub fn wipe(&mut self) -> Result<&mut Self, Error> { - self.wipe_method(|conn| unsafe { s2n_connection_wipe(conn.as_ptr()).into_result() })?; - // we deliberately call this outside of "wipe_method", because binding - // specific defaults should not be re-applied on renegotiate wipe - self.set_binding_specific_defaults()?; + // s2n_connection_wipe is a nightmare of a method, with lifetime issues + // that are incredibly difficult to reason about. We do not expose it in + // the rust bindings. In our benchmarking, the savings were ~ 2 us, which + // is less than 1% of the cost of a handshake. + let clean_connection = { + let mut connection = Connection::new(self.mode()); + // config will be none if Connection::set_config has yet to be called + if let Some(config) = self.config() { + connection.set_config(config)?; + } + connection + }; + *self = clean_connection; + Ok(self) } @@ -1975,6 +1988,7 @@ mod tests { /// Confirm that the large (16KB) record size is used by both newly /// created connections and wiped (reused) connections. #[test] + #[allow(deprecated)] fn max_record_size_configuration() -> Result<(), Box> { /// https://www.rfc-editor.org/info/rfc8446/#section-5.1 /// > The length MUST NOT exceed 2^14 bytes. @@ -2070,6 +2084,7 @@ mod tests { /// `wipe` preserves the mode (client/server) of the connection. #[test] + #[allow(deprecated)] fn wipe_preserves_mode() -> Result<(), Box> { let mut client = Connection::new_client(); client.wipe()?; @@ -2083,6 +2098,7 @@ mod tests { /// `wipe` preserves the config set on the connection. #[test] + #[allow(deprecated)] fn wipe_preserves_config() -> Result<(), Box> { use crate::connection::Builder; @@ -2101,6 +2117,7 @@ mod tests { /// `wipe` clears any application context stored on the connection. #[test] + #[allow(deprecated)] fn wipe_clears_application_context() -> Result<(), Box> { let mut conn = Connection::new_server(); @@ -2116,6 +2133,7 @@ mod tests { /// A wiped connection can be reused for a subsequent handshake. #[test] + #[allow(deprecated)] fn wipe_allows_connection_reuse() -> Result<(), Box> { // arbitrary policy. This test has no specific parameter expectations let config = build_config(&security::DEFAULT)?; diff --git a/bindings/rust/extended/s2n-tls/src/connection/builder.rs b/bindings/rust/extended/s2n-tls/src/connection/builder.rs index 6d637d68fa3..6a761e37605 100644 --- a/bindings/rust/extended/s2n-tls/src/connection/builder.rs +++ b/bindings/rust/extended/s2n-tls/src/connection/builder.rs @@ -1,13 +1,9 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - config::Config, - connection::Connection, - enums::Mode, - error::Error, - pool::{Pool, PooledConnection}, -}; +#[allow(deprecated)] +use crate::pool::{Pool, PooledConnection}; +use crate::{config::Config, connection::Connection, enums::Mode, error::Error}; /// A trait indicating that a structure can produce connections. pub trait Builder: Clone { @@ -26,6 +22,7 @@ impl Builder for Config { } /// Produces new connections from a pool of reuseable connections. +#[allow(deprecated)] impl Builder for T { type Output = PooledConnection; fn build_connection(&self, mode: Mode) -> Result { @@ -72,6 +69,7 @@ where } #[cfg(test)] +#[allow(deprecated)] mod tests { use super::*; use crate::pool::ConfigPoolBuilder; diff --git a/bindings/rust/extended/s2n-tls/src/pool.rs b/bindings/rust/extended/s2n-tls/src/pool.rs index 209e1c21438..f90a2b8e3fa 100644 --- a/bindings/rust/extended/s2n-tls/src/pool.rs +++ b/bindings/rust/extended/s2n-tls/src/pool.rs @@ -1,20 +1,13 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -//! Utilities to handle reusing connections. -//! -//! Creating a single new connection requires significant -//! memory allocations (about 50-60 KB, according to some tests). -//! Instead of allocating memory for a new connection, existing -//! memory can be reused by calling -//! [Connection::wipe()](`crate::connection::Connection::wipe()). +// This module implements deprecated pool functionality, so internal usage of +// deprecated items is expected. +#![allow(deprecated)] + +//! Deprecated utilities to handle reusing connections. //! -//! On modern systems with reasonably performant allocators, the benefits of reusing -//! connections are reduced. Connection reuse is specifically intended for customers -//! who are sensitive to allocations or for whom allocations are more expensive. -//! Customers are encouraged to run their own benchmarks to determine the exact -//! performance benefit. As a starting point, a simple benchmark comparing allocation -//! against reuse can be found `bench/benches/connection_creation.rs`. +//! [`Connection::wipe`] //! //! The [`Pool`] trait allows applications to define an //! [Object pool](https://en.wikipedia.org/wiki/Object_pool_pattern) that @@ -41,6 +34,9 @@ use std::{ /// When dropped, returns ownership of the connection to /// the pool that produced it by calling [`Pool::give`]. #[derive(Debug)] +#[deprecated( + note = "use `Connection::new()` instead; connection reuse provides negligible performance benefit" +)] pub struct PooledConnection> { pool: T, conn: Option, @@ -94,6 +90,9 @@ impl PooledConnection { /// /// Minimally, an implementation should call [`Connection::wipe()`] /// during [`Self::give`]. +#[deprecated( + note = "use `Connection::new()` instead; connection reuse provides negligible performance benefit" +)] pub trait Pool { fn mode(&self) -> Mode; fn take(&self) -> Result; @@ -131,6 +130,9 @@ impl Pool for Arc { /// /// For discussions about expected performance benefits see [self]. #[derive(Debug)] +#[deprecated( + note = "use `Connection::new()` instead; connection reuse provides negligible performance benefit" +)] pub struct ConfigPool { mode: Mode, config: Config, @@ -138,9 +140,15 @@ pub struct ConfigPool { max_pool_size: usize, } +#[deprecated( + note = "use `Connection::new()` instead; connection reuse provides negligible performance benefit" +)] pub type ConfigPoolRef = Arc; /// Builder for [`ConfigPool`]. +#[deprecated( + note = "use `Connection::new()` instead; connection reuse provides negligible performance benefit" +)] pub struct ConfigPoolBuilder(ConfigPool); impl ConfigPoolBuilder { pub fn new(mode: Mode, config: Config) -> Self { diff --git a/bindings/rust/standard/benchmarks/Cargo.toml b/bindings/rust/standard/benchmarks/Cargo.toml index 6442071af52..f774a8e652b 100644 --- a/bindings/rust/standard/benchmarks/Cargo.toml +++ b/bindings/rust/standard/benchmarks/Cargo.toml @@ -28,7 +28,3 @@ harness = false [[bench]] name = "resumption" harness = false - -[[bench]] -name = "connection_creation" -harness = false diff --git a/bindings/rust/standard/benchmarks/benches/connection_creation.rs b/bindings/rust/standard/benchmarks/benches/connection_creation.rs deleted file mode 100644 index c2a2d78741a..00000000000 --- a/bindings/rust/standard/benchmarks/benches/connection_creation.rs +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -use criterion::{criterion_group, criterion_main, Criterion}; -use s2n_tls::{ - config::Config, - connection::Builder, - enums::Mode, - pool::{ConfigPool, ConfigPoolBuilder, PooledConnection}, -}; -use std::sync::Arc; - -fn connection_wipe(connection_pool: &Arc) { - // get a connection from the pool - let conn = PooledConnection::new(connection_pool).unwrap(); - // "drop" the connection, wiping it and returning it to the pool - drop(conn); -} - -fn connection_new(config: &Config) { - let conn = config - .build_connection(s2n_tls::enums::Mode::Server) - .unwrap(); - drop(conn); -} - -fn connection_creation(c: &mut Criterion) { - let mut group = c.benchmark_group("Connection Creation"); - let config = s2n_tls::config::Builder::new().build().unwrap(); - let connection_pool = ConfigPoolBuilder::new(Mode::Server, config.clone()).build(); - - group.bench_function("connection reuse", |b| { - b.iter(|| connection_wipe(&connection_pool)); - }); - - group.bench_function("connection allocation", |b| { - b.iter(|| connection_new(&config)); - }); - - group.finish(); -} - -criterion_group!(benches, connection_creation); -criterion_main!(benches);