Fix Send and Sync bounds for M to avoiding unsound behavior.

[ram-nad]

Hi,

Not sure if this is right place to ask question, please excuse me if I am at wrong place. I am relatively new to Rust and am trying to understand some intricacies of the language.

I was reading through the code and am curious if we need the following,

unsafe impl<M: Send + Sync> Sync for Runnable<M> {}

Why can't it just be

unsafe impl<M: Sync> Sync for Runnable<M> {}

As per my understanding, even if M is not Send, the Task and Runnable can still be safely shared across threads because there is no way to get exclusive access of metadata through their reference.

[notgull]

I'm following the same rules as Arc in this case, where Arc<T> is Sync only if T is Send + Sync.

See: https://doc.rust-lang.org/std/sync/struct.Arc.html#impl-Sync-for-Arc%3CT,+A%3E

[ram-nad]

Hi @notgull Thanks for the reply.

I am seeing this paragraph in Rust Atomics and Locks book.

Sending an Arc across threads results in a T object being shared, requiring T to be Sync. Similarly, sending an Arc across threads could result in another thread dropping that T, effectively transferring it to the other thread, requiring T to be Send. In other words, Arc should be Send if and only if T is both Send and Sync. The exact same holds for Sync, since a shared &Arc can be cloned into a new Arc.

But same doesn't hold true for Runnable/Task. There is no way to obtain them for their references.

[notgull]

It's possible for a Runnable and a Task (both of which allow access to M) to be on different threads.

[ram-nad]

I understand that, but I am still not sure on how that translates to the bound requirements. Even if M is just Sync and !Send, it should not prevent Runnable/Task from being marked as Sync since both Task and Runnable only allow shared access to M.

[notgull]

The following sequence of events can occur:

• Task and Runnable is created on thread 1 with M.
• Runnable is sent to thread 2.
• Task is dropped on thread 1.
• Runnable is dropped on thread 2. Since it's the last reference, it drops the M.

Then the drop handler has a mutable reference to M on thread 2. This can also happen with wakers.

[ram-nad]

Hi @notgull,

Thanks so much again for patient replies. I have already thought of this case. It doesn't conflict with the change that I am trying to propose.

Runnable is sent to thread 2: This can only happen when Runnable<M>: Send which can only happen if M: Send + Sync. Since M: Send, it will be okay to drop it in thread 2.

The change I am proposing is following,

unsafe impl<M: Send + Sync> Send for Runnable<M> {}
unsafe impl<M: Sync> Sync for Runnable<M> {}

unsafe impl<T: Send, M: Send + Sync> Send for Task<T, M> {}
unsafe impl<T, M: Sync> Sync for Task<T, M> {}

I don't seem to find any reason for it to not be like this.

Thanks again for taking time to explain things to me.

Edit: Fixed minor error in the code block

[notgull]

Note that the wakers are always Send and Sync... which now that I say this, implies that M should always be Sene, shouldn't it?

[ram-nad]

@notgull, Yes I agree. We should mark the requirement of M: Sync + Send ideally in spawn and spawn_local APIs. I would still think that change proposed here should still be valid for the unsafe unchecked API.

[nurmohammed840]

I think, we can obtain &mut M from Runable,
Metadata should not be accessible from JoinHandler aka. Task

Unfortunately, it can't be change now.

[ram-nad]

@nurmohammed840, does current implementation allow us to access &mut M from Runnable?

[nurmohammed840]

With current API ? no.

However, We could add additional field M2, which will not be shared. and owned by Runnable exclusively.
Because Runnable owned the data, we can access &mut M2 from Runnable.

I am working on a similar library (in development), which is not publish yet.

[ram-nad]

Note that the wakers are always Send and Sync... which now that I say this, implies that M should always be Sene, shouldn't it?

@notgull, do we have any update on this? I think this could be a unwanted behavior in current implementation. For example, this could allow safe code to access Rc from multiple threads leading to data races.

1. If M was an Rc
2. We spawn a future in Thread 1.
3. When the future is being polled for first time, we send its Waker to Thread 2
4. We still have access to Task in Thread 1
5. When we call wake for the Waker in Thread 2, we will have get back a Runnable in Thread 2 itself
6. We have access to &M in both Thread 1 and Thread 2

Please let me know if you have any thoughts on this.

[nurmohammed840]

@ram-nad, What is your scenario for using M?

If M was an Rc

Rc is not Send nor Sync. Therefor can't be used.

We still have access to Task in Thread 1

What do you mean by accessing Task in Thread 1?

When we call wake for ... a Runnable in Thread 2 itself.

Scheduler is like a call back function . When Waker is called Thread 2. Scheduler function is called in that context.

[ram-nad]

@nurmohammed840, I am just pointing out a case in which current implementation allows us to safely access Rc in multiple threads. async-task allows us to access metadata through both Task and Runnable objects returned from spawn function.

Also, there is no restriction in the APIs requiring M to be either Send or Sync.

Therefore, if we follow above listed steps, we can end up getting access to a Rc variable in multiple threads which as per my understanding should be not possible in safe code.

[nurmohammed840]

Huh, That's interesting.
The problem is that, Waker is both Send and Sync. and creating Waker from runable.waker() don't require M to be Send.

So this code is allowed.

use async_task::Builder;
use std::{rc::Rc, thread};

let rc = Rc::new(42);
let (runable, _) = Builder::new().metadata(rc).spawn(|_| async {}, |_| {});
let waker = runable.waker();
thread::spawn(move || {
    waker.wake();
});

Although it's an easy fix,

impl<M: Send> Runnable<M> {
// or
pub fn waker(&self) -> Waker 
where 
    M: Send
{ ... }

Disallow creating non Send able M in spawn function.