sancane / rustun

Rust implementation for Session Traversal Utilities for NAT (STUN)
Apache License 2.0
9 stars 0 forks source link

Working with async move tokio tasks? #29

Closed rageshkrishna closed 7 months ago

rageshkrishna commented 7 months ago

I'm trying to use MessageBuilder inside an async move tokio task that calls async code inside it, but if there's an .await after instantiating a MessageBuilder, the compiler complains that the future is not safe to share between threads because an Rc inside StunAttribute doesn't implement Send.

Here's the snippet that triggers the violation:

let handle = tokio::spawn(async move {
    loop {
        let mut in_buffer = vec![0; 1024];
        eprintln!("Waiting for message");
        let recvd = socket.recv_from(&mut in_buffer).await.unwrap();
        eprintln!("Received {} from {}", recvd.0, recvd.1);

        // Removing this block of code _or_ the async call beneath will remove the compiler error
        let msg = StunMessageBuilder::new(BINDING, MessageClass::SuccessResponse).build();
        let encoder = MessageEncoderBuilder::default().build();
        let mut out_buffer = vec![0; 512];
        let size = encoder.encode(&mut out_buffer, &msg).unwrap();

        // Some async stuff happens here...
        socket.send_to(&res.clone(), recvd.1).await.unwrap();

The compiler error is:

error: future cannot be sent between threads safely
   --> src\stun_client.rs:63:22
    |
63  |         let handle = tokio::spawn(async move {
    |                      ^^^^^^^^^^^^ future created by async block is not `Send`
    |
    = help: within `stun_rs::StunAttribute`, the trait `std::marker::Send` is not implemented for `std::rc::Rc<[u8; 32]>`
note: future is not `Send` as this value is used across an await
   --> src\stun_client.rs:76:55
    |
70  |                 let msg = StunMessageBuilder::new(BINDING, MessageClass::SuccessResponse).build();
    |                     --- has type `stun_rs::StunMessage` which is not `Send`
...
76  |                 socket.send_to(&res.clone(), recvd.1).await.unwrap();
    |                                                       ^^^^^ await occurs here, with `msg` maybe used later
note: required by a bound in `tokio::spawn`
   --> R:\Ragesh\packages\cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.36.0\src\task\spawn.rs:166:21
    |
164 |     pub fn spawn<F>(future: F) -> JoinHandle<F::Output>
    |            ----- required by a bound in this function
165 |     where
166 |         F: Future + Send + 'static,
    |                     ^^^^ required by this bound in `spawn`

Any suggestions on how to work around this?

sancane commented 7 months ago

Hi, effectively, stun-rs library is designed to minimice the number of memory allocations, that's why some attributes uses RC to keep the same memory references among copies of attributes. Unlike Arc, Rc is ligther compared to atomic reference counters, but this makes it single-threaded. That's why the compier is complaining when you try to move it among async tasks. To be honest, I never felt the need to mve to ARC on my async code with stun-rs, given that most of the times there is no need for such a thing, you can fix that just limiting the scope of the builder in your code, let me give you an example:

let handle = tokio::spawn(async move {
        let mut out_buffer = vec![0; 512];

        {
            let software = Software::new("STUN test client").expect("Could not create Software attribute");
            let password = "VOkJxbRl1RmTxUk/WvJxBt";
            let key = HMACKey::new_short_term(password).expect("Could not create HMACKey");
            let integrity = MessageIntegrity::new(key);
            let fingerprint = Fingerprint::default();

            let msg = StunMessageBuilder::new(BINDING, MessageClass::Request)
                .with_attribute(software)
                .with_attribute(integrity)
                .with_attribute(fingerprint)
                .build();

            let encoder = MessageEncoderBuilder::default().build();
            let size = encoder.encode(&mut out_buffer, &msg).unwrap();
        }

        // Async task can be called now
        do_something().await;
        println!("Done");
    });

Of course, if this turns into a recurrent problem, I could change it to be Send friendly, I'm open to requests. Please feel free to report any other issue you hit that helps me make a decision about turning it into ARC ref counters instead. Best Regards

rageshkrishna commented 7 months ago

Thanks for the update. Limiting scope with blocks is what I ended up doing as well to get around this, the only drawback being that I need to add comments to explain why 😄. I think this is OK for me at the moment. Not much point in paying the Arc tax if it isn't absolutely necessary.

rageshkrishna commented 7 months ago

BTW, thank you for the great module! It gave me everything I needed to work with STUN and it was really easy to get started by just looking at the readme.

sancane commented 7 months ago

I usually encapsule the code to generate messages in functions, that' helps with code readability and maintenance. Thanks for the feedback. I really appreciate it. Don't hesitate to contact me for any issue you may experiment.