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

Force Aead::AeadImpl to be Send + Sync #61

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Conversation

nox
Copy link
Contributor

@nox nox commented Jun 7, 2024

When trying to use AeadCtxR/AeadCtxS in a generic context which requires Send or Sync, downstream users need to add a bound to Aead::AeadImpl even though this is a doc(hidden) item and thus an implementation detail that may change afterwards.

All implementations of Aead provide an AeadImpl type that is already Send + Sync so it doesn't hurt to add a trait bound there.

When trying to use AeadCtxR/AeadCtxS in a generic context which requires
Send or Sync, downstream users need to add a bound to Aead::AeadImpl
even though this is a doc(hidden) item and thus an implementation detail
that may change afterwards.

All implementations of Aead provide an AeadImpl type that is already
Send + Sync so it doesn't hurt to add a trait bound there.
@rozbb
Copy link
Owner

rozbb commented Jun 9, 2024

Thank you for bringing this up! Do you have a minimal example by any chance that works after this PR but not before? I was trying to see how far you could get with AeadCtx in threads after this PR, and I can't make it work.

I added the following code to examples/client_server.rs

fn do_export<A, K1, K2>(ctx: AeadCtxR<A, K1, K2>)
where
    A: hpke::aead::Aead,
    K1: hpke::kdf::Kdf,
    K2: hpke::kem::Kem + Send + Sync,
{
    // Wrap the context to prevent lifetime issues
    let wrapped = std::rc::Rc::new(ctx);
    // Do an export() (which doesn't require mut) in another thread
    std::thread::spawn(move || {
        let mut buf = [0u8; 32];
        wrapped.export(b"foobarcontext", &mut buf);
    });
}

And I get the following error


error[E0277]: `Rc<AeadCtxR<A, K1, K2>>` cannot be sent between threads safely
   --> examples/client_server.rs:110:24
    |
110 |       std::thread::spawn(move || {
    |       ------------------ ^------
    |       |                  |
    |  _____|__________________within this `{closure@examples/client_server.rs:110:24: 110:31}`
    | |     |
    | |     required by a bound introduced by this call
111 | |         let mut buf = [0u8; 32];
112 | |         wrapped.export(b"foobarcontext", &mut buf);
113 | |     });
    | |_____^ `Rc<AeadCtxR<A, K1, K2>>` cannot be sent between threads safely
    |

@nox
Copy link
Contributor Author

nox commented Jun 11, 2024

Rc<T> is never Send, you want Arc<T>:

fn do_export<A, K1, K2>(ctx: hpke::aead::AeadCtxR<A, K1, K2>)
where
    A: hpke::aead::Aead + 'static,
    K1: hpke::kdf::Kdf + 'static,
    K2: hpke::kem::Kem + Send + Sync + 'static,
{
    // Wrap the context to prevent lifetime issues
    let wrapped = std::sync::Arc::new(ctx);
    // Do an export() (which doesn't require mut) in another thread
    std::thread::spawn(move || {
        let mut buf = [0u8; 32];
        wrapped.export(b"foobarcontext", &mut buf);
    });
}

@rozbb rozbb merged commit af13275 into rozbb:main Jul 3, 2024
11 checks passed
@rozbb
Copy link
Owner

rozbb commented Jul 3, 2024

Apologies for the delay. Thank you!

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

Successfully merging this pull request may close these issues.

2 participants