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.

Sign up for GitHub

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

Jump to bottom

Replace use of crossbeam_channel with std::sync::mpsc #7153

Closed
james7132 opened this issue Jan 10, 2023 · 7 comments · May be fixed by #13048
Closed

Replace use of crossbeam_channel with std::sync::mpsc #7153

james7132 opened this issue Jan 10, 2023 · 7 comments · May be fixed by #13048
Labels
C-Dependencies A change to the crates that Bevy depends on S-Blocked This cannot move forward until something else changes

Comments

@james7132
Copy link
Member

What problem does this solve or what need does it fill?

In Rust 1.67, a vendored implementation of crossbeam_channel will be used to back std::sync::mpsc. This was added in rust-lang/rust#93563.

What solution would you like?

When 1.67 lands, use std::sync::mpsc instead of crossbeam_channel in single-producer/single-consumer and multiple-producer/single-consumer use cases, after bumping the MSRV. Potentially shrink the dependency tree.

What alternative(s) have you considered?

Continue using crossbeam_channel as is.
@james7132 james7132 added C-Dependencies A change to the crates that Bevy depends on S-Blocked This cannot move forward until something else changes labels Jan 10, 2023
@alice-i-cecile alice-i-cecile added the D-Trivial Nice and easy! A great choice to get started with Bevy label Jan 10, 2023
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 10, 2023 •
edited
Loading

This will be unblocked on January 26th, 2023. At that point, this should be straightforward to tackle.

If you want to start on a PR before then, prepare it by swapping your compiler version to nightly.

@harudagondi
Copy link
Member

I think it should be beta, not nightly.

@OneFourth
Copy link
Contributor

Hi, I wanted to try this as a first issue but I think there might be a problem. As mentioned here

bevy/examples/async_tasks/external_source_external_thread.rs

Line 4 in d4babaf

// Using crossbeam_channel instead of std as std `Receiver` is `!Sync`

Since Resource requires Sync there's a few errors when building bevy_asset and bevy_time (such as std::sync::mpsc::Receiver<Instant> cannot be shared between threads safely). Sorry, I don't understand Send and Sync all too well, so I'm not sure if there's a proper workaround or if this is just fundamentally incompatible with the std implementation.

@james7132
Copy link
Member Author

james7132 commented Jan 10, 2023 •
edited
Loading

Ah the good ol' problem of !Sync components and resources. Sync requires that &T be safe to send to another thread, but that says nothing about &mut T, which is always safe to send to another thread provided the type is Send, which the receiver is . You could try using bevy_utils::sync_cell::SyncCell to wrap the reciever, which will make it Sync but only allow you to get a mutable borrow to read it, which I think should be fine for both bevy_asset and bevy_time.

@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Jan 10, 2023
@OneFourth
Copy link
Contributor

So I bit off more than I can chew and gave this a try anyway, bevy_asset in particular is a rabbit hole of build errors trying to get it to work, the other crates should be ok I think.

Can I open a pull request for what I've got so far? I've still got some compile errors that I can't figure out. Or should I just give up on trying to do this one?

@alice-i-cecile
Copy link
Member

@OneFourth A draft PR would be super useful, even if you immediately close it :) Link this issue from there so others can see what you've got.

@OneFourth OneFourth mentioned this issue Jan 23, 2023
Closed
@james7132
Copy link
Member Author

As mentioned in #7346, this seems impossible or generally undesirable with the current stable interface of std::sync::mpsc. Closing.

@alice-i-cecile alice-i-cecile closed this as not planned Won't fix, can't repro, duplicate, stale Sep 17, 2023
@james7132 james7132 mentioned this issue Apr 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-Dependencies A change to the crates that Bevy depends on S-Blocked This cannot move forward until something else changes
Projects
None yet
Milestone
No milestone
4 participants
@OneFourth @james7132 @alice-i-cecile @harudagondi