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

Can't use ring channel in async context: NonNull<ControlBlock> is not Send` #69

Closed
niklasad1 opened this issue Nov 24, 2020 · 8 comments · Fixed by #91
Closed

Can't use ring channel in async context: NonNull<ControlBlock> is not Send` #69

niklasad1 opened this issue Nov 24, 2020 · 8 comments · Fixed by #91

Comments

@niklasad1
Copy link

niklasad1 commented Nov 24, 2020

Hey, thanks for this crate.

However, I'm trying to use ring channel where it's spawned in a background thread and when I try to spawn futures on I get:

99  |                     let task = rt.spawn(async move {
    |                                   ^^^^^ future created by async block is not `Send`
    |
    = help: within `Foo`, the trait `Sync` is not implemented for `NonNull<ring_channel::control::ControlBlock<Bar>>`
note: captured value is not `Send`

Is it reasonable to replace NonNull with AtomicPointer?

@brunocodutra
Copy link
Owner

Hey, glad you find it useful!

It might actually be safe to implement Sync for the channel, I'd have to think about it, but I wonder why you wouldn't simply clone it? Is there a particular reason why you would prefer to share it across thread boundaries?

@niklasad1
Copy link
Author

niklasad1 commented Nov 25, 2020

Right, the way I'm using it clone doesn't help AFAIU

Example for what I'm trying to:

use futures::prelude::*;
use futures::sink::SinkExt;
use ring_channel::*;
use std::num::NonZeroUsize;

#[derive(Clone)]
struct A {
    backend: RingSender<()>,
}

impl A {
    fn new() -> Self {
        let (tx, rx) = ring_channel(NonZeroUsize::new(1).unwrap());
        async_std::task::spawn(async move {
            background_consumer(rx).await;
        });
        Self { backend: tx }
    }

    async fn push(&self, t: ()) -> Result<(), ()> {
        let mut backend = self.backend.clone();
        SinkExt::send(&mut backend, t).await.map_err(|_e| ())
    }
}

#[async_std::main]
async fn main() {
    let a = A::new();

    let a_rc = a.clone();
    async_std::task::spawn(async move {
        let _ = a_rc.push(()).await.unwrap();
    });

    loop {}
}

async fn background_consumer(mut consumer: RingReceiver<()>) {
    loop {
        let msg = consumer.next().await;
        println!("{:?}", msg);
    }
}

@loafofpiecrust
Copy link

loafofpiecrust commented Nov 24, 2021

I'm trying to use ring-channel in a similar context: where RingReceiver<_> is a struct field and I want to give that struct some async methods and stick it into an Arc<_>. Right now, the code fails to compile because RingReceiver<_> is !Sync. This feels pretty important for building structures on top of ring-channel. Coming from flume or mpsc this is a loss. Any plans to consider this change further?

@brunocodutra
Copy link
Owner

Hey @loafofpiecrust, thanks for bringing my attention back to this issue! I'll have a look at this over the weekend.

@brunocodutra
Copy link
Owner

I haven't forgotten about this issue, just haven't managed to find the time to work on it yet, but it's still at the top of my backlog.

@loafofpiecrust
Copy link

@brunocodutra Thanks for the follow-up. I've been using my fork with a change based on what's linked earlier in this thread, using atomics. Here's my branch, though I'm not sure if it's the perfect approach. Should I make a PR?

brunocodutra added a commit that referenced this issue Dec 15, 2021
This reverts commit 972691acdb3200df8a9ef9b0d687357e7cb5535f972691 and closes #69.
brunocodutra added a commit that referenced this issue Dec 15, 2021
@brunocodutra
Copy link
Owner

@loafofpiecrust it's actually safe to impl Sync for RingSender and RingReceiver, no need to use an atomic pointer, see #91.

brunocodutra added a commit that referenced this issue Dec 15, 2021
@brunocodutra
Copy link
Owner

@niklasad1 & @loafofpiecrust I'm sorry it took so long, but I've just published v0.10 to solve this issue, please check it out when you have the chance and reopen this issue in case you still face problems with this use-case.

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 a pull request may close this issue.

3 participants