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.

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

docs: more Send and Sync safety documentation #4471

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Mar 25, 2024

Resolved issues:

resolves #4413

Description of changes:

I try to explain why structs aren't automatically Send / Sync and why it's safe to mark them Send / Sync anyway. I'm trying not to explain Send / Sync, since existing Rust documentation already does that and that shouldn't be the responsibility of our docs. On the other hand, I want developers who haven't read that documentation to still have an idea how not to break our Send / Sync safety.

Testing:

Mostly documentation changes, with Sync added to some certificate structs.

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

@github-actions github-actions bot added the s2n-core team label Mar 25, 2024
@lrstewart lrstewart marked this pull request as ready for review March 25, 2024 08:30
@@ -61,9 +61,56 @@ impl fmt::Debug for Connection {

/// # Safety
///
/// s2n_connection objects can be sent across threads
/// NonNull / the raw s2n_connection pointer isn't Send because its data may be
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a note in general for your doc comments. It's not important to know why the raw pointer isn't Send/Sync. I would prefer it if you started out with why the Connection/Config/CertChain is Send/Sync. If you need to elaborate on why the raw s2n_connection pointer isn't Send/Sync, maybe do that after? But I think most people are just looking for why this Rust struct was tagged with Send/Sync.

So with that in mind, I would probably restructure your comment like:
The Connection is Send because the existing interface ensures that only one owned Connection can exist for each s2n_connection C object.
The raw s2n_connection pointer it wraps is not Send, and therefore library developers MUST ensure that new methods do not expose the raw s2n_connection pointer...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: Rust bindings need an explanation for Send/Sync impls
2 participants