feat(s2n-quic-core) TLS offloading #2688

maddeleine · 2025-07-01T01:29:15Z

Release Summary:

Adds unstable feature to move s2n-tls operations out of the main thread.

Resolved issues:

resolves #2598

Description of changes:

Adds a new TLS provider that opens a new thread to complete TLS operations. This is essentially what was implemented by Mark in this diff except cleaned up a little.

Call-outs:

Testing:

Adds integ test

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

WesleyRosenblum · 2025-07-07T20:59:09Z

quic/s2n-quic-core/Cargo.toml

+futures-channel = { version = "0.3", default-features = false, optional=true, features = ["std", "alloc"]}
+futures = { version = "0.3", default-features = false, optional=true, features = ["std", "alloc"]}


looks like there was an intention to have the dependencies be alphabetically ordered (through the last two in the list broke that). Could you move these and future-test and once_cell?

We also already have a channel in https://github.com/aws/s2n-quic/blob/main/quic/s2n-quic-core/src/sync/spsc.rs.

WesleyRosenblum · 2025-07-07T21:22:49Z

quic/s2n-quic-core/Cargo.toml

 default = ["alloc", "std"]
 alloc = ["atomic-waker", "bytes", "crossbeam-utils", "s2n-codec/alloc"]
-std = ["alloc", "once_cell"]
+std = ["alloc", "once_cell", "futures-channel", "futures"]


I wondering if it would make more sense to have futures-channel and futures be part of the unstable-offload-tls feature in this crate. Since you don't really need those dependencies if you are only interested in using std and not the TLS offloading feature

WesleyRosenblum · 2025-07-07T21:26:12Z

quic/s2n-quic-core/src/crypto/tls.rs

 #[cfg(feature = "alloc")]
 pub mod slow_tls;

+#[cfg(feature = "std")]


Suggested change

#[cfg(feature = "std")]

#[cfg(feature = "unstable-offload-tls")]

WesleyRosenblum · 2025-07-07T21:53:25Z

quic/s2n-quic-core/src/crypto/tls/offload.rs

+);
+pub struct OffloadEndpoint<E: tls::Endpoint> {
+    new_session: UnboundedSender<SessionProducer<E>>,
+    _thread: thread::JoinHandle<()>,


is this needed?

WesleyRosenblum · 2025-07-07T21:59:14Z

quic/s2n-quic-core/src/crypto/tls/offload.rs

+
+                let mut next_sessions = vec![];
+
+                // Make progress on all stored sessions, prioritizing existing sessions over incoming ones


can you clarify a bit on what you mean by "prioritizing existing sessions over incoming ones" and how that relates to sessions and next_sessions?

WesleyRosenblum · 2025-07-07T22:16:51Z

quic/s2n-quic-core/src/crypto/tls/offload.rs

+                        // FIXME: this probably means that the parent thread is no longer interested
+                        // in this connection and we should instead tear it down.


can this be fixed now or is there more investigation needed? If it can't be fixed now, would be good to link an issue

WesleyRosenblum · 2025-07-07T22:17:59Z

quic/s2n-quic-core/src/crypto/tls/offload.rs

+        &mut self,
+        _session: &impl tls::TlsSession,
+    ) -> Result<(), crate::transport::Error> {
+        // FIXME: needs some form of async callback, or maybe never gets called during remote phase?


same for this

WesleyRosenblum · 2025-07-07T22:19:03Z

quic/s2n-quic-core/src/crypto/tls/offload.rs

+        }) {
+            resp
+        } else {
+            // FIXME: either async-ify, remove, or figure out what the Pending value should be.


what is needed to know the fix?

there shouldn't be any async communication in these methods. it should just simplify check if there is capacity in the queue and if the space is available.

WesleyRosenblum · 2025-07-07T22:19:45Z

quic/s2n-quic/Cargo.toml

 # The feature enables the close formatter provider
 unstable-provider-connection-close-formatter = []
+# This feature enables the use of the offloaded TLS feature
+unstable-offload-tls = []


I think this would end up being

Suggested change

unstable-offload-tls = []

unstable-offload-tls = ["s2n-quic-core/unstable-offload-tls"]

WesleyRosenblum · 2025-07-07T22:21:45Z

quic/s2n-quic/src/tests/offload_tls.rs

+
+#[test]
+#[cfg(feature = "unstable-offload-tls")]
+fn offload_tls() {


looks pretty straightforward to use, but might still be worth an example in the examples directory. And that can contain some documentation on the feature in a readme

camshaft · 2025-07-07T22:46:20Z

quic/s2n-quic-core/src/crypto/tls/offload.rs

    pub fn new(inner: E) -> Self {
        let (tx, mut rx) = futures_channel::mpsc::unbounded::<SessionProducer<E>>();

        let handle = thread::spawn(move || {


I don't think we want to spawn a new thread per session. This will essentially be an unbounded queue of spawning threads.

camshaft · 2025-07-07T22:48:15Z

quic/s2n-quic/src/tests/offload_tls.rs

+        .unwrap();
+    let server_endpoint = Offload(server_endpoint);
+    let client_endpoint = Offload(client_endpoint);
+    test(model, |handle| {


I'm surprised this is working with bach. I won't like interacting with an actual async operation (interacting with a thread). My guess is it's a race condition of things actually working or not, at the very least non-deterministic.

camshaft · 2025-07-07T22:49:29Z

quic/s2n-quic-core/Cargo.toml

+futures-channel = { version = "0.3", default-features = false, optional=true, features = ["std", "alloc"]}
+futures = { version = "0.3", default-features = false, optional=true, features = ["std", "alloc"]}


We also already have a channel in https://github.com/aws/s2n-quic/blob/main/quic/s2n-quic-core/src/sync/spsc.rs.

camshaft · 2025-07-07T22:50:10Z

quic/s2n-quic-core/src/crypto/tls.rs

    fn receive_application(&mut self, max_len: Option<usize>) -> Option<Bytes>;

-    fn can_send_initial(&self) -> bool;
+    fn can_send_initial(&mut self) -> bool;


this is technically a breaking change if applications are implementing their own TLS providers.

camshaft · 2025-07-07T22:52:37Z

quic/s2n-quic-core/src/crypto/tls/offload.rs

+        remote_thread: Waker,
+    ) -> Self {
+        // Channel to pass requests from remote TLS thread to main thread
+        let (tx, rx) = futures_channel::mpsc::unbounded::<Request<S>>();


we really want to avoid any kind of unbounded queues

camshaft · 2025-07-07T22:54:41Z

quic/s2n-quic-core/src/crypto/tls/offload.rs

+        }) {
+            resp
+        } else {
+            // FIXME: either async-ify, remove, or figure out what the Pending value should be.


there shouldn't be any async communication in these methods. it should just simplify check if there is capacity in the queue and if the space is available.

maddeleine added 2 commits June 30, 2025 18:14

Initial commit

0b1dd6f

Clippy lints

0c9d359

maddeleine changed the title ~~Initial commit~~ feat(s2n-quic-core) TLS offloading Jul 1, 2025

maddeleine added 4 commits June 30, 2025 21:39

no-std fixes

99968fb

Adds test

baabea9

Remove std from core

06a2100

Clippy

1bf7f1e

maddeleine marked this pull request as ready for review July 7, 2025 17:45

maddeleine requested a review from WesleyRosenblum July 7, 2025 17:45

WesleyRosenblum reviewed Jul 7, 2025

View reviewed changes

camshaft reviewed Jul 7, 2025

View reviewed changes

maddeleine mentioned this pull request Jul 18, 2025

feat(s2n-quic-core) TLS offloading #2721

Merged

maddeleine closed this Jul 18, 2025

		futures-channel = { version = "0.3", default-features = false, optional=true, features = ["std", "alloc"]}
		futures = { version = "0.3", default-features = false, optional=true, features = ["std", "alloc"]}

	#[cfg(feature = "std")]
	#[cfg(feature = "unstable-offload-tls")]


		let mut next_sessions = vec![];

		// Make progress on all stored sessions, prioritizing existing sessions over incoming ones

		// FIXME: this probably means that the parent thread is no longer interested
		// in this connection and we should instead tear it down.

	unstable-offload-tls = []
	unstable-offload-tls = ["s2n-quic-core/unstable-offload-tls"]

feat(s2n-quic-core) TLS offloading #2688

feat(s2n-quic-core) TLS offloading #2688

Uh oh!

Conversation

maddeleine commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Summary:

Resolved issues:

Description of changes:

Call-outs:

Testing:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maddeleine commented Jul 1, 2025 •

edited

Loading