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

The result of serve_nbd is never Send #11

Open
refi64 opened this issue Jan 9, 2022 · 4 comments
Open

The result of serve_nbd is never Send #11

refi64 opened this issue Jan 9, 2022 · 4 comments

Comments

@refi64
Copy link

refi64 commented Jan 9, 2022

#4's first commit removed the Send requirement on BlockDevice. This makes sense, because there's no real reason it needs to be send...but it also has a very unfortunate consequence: BlockDevice can never be Send.

As a result, serve_nbd's returned Future isn't Send, which heavily limits where it can be used. In particular, I was trying to do this:

tokio::spawn(start_nbd(...));

But, because start_nbd's result isn't Send, it's not possible without resorting to using a LocalSet, which is...well, pretty ugly.

Unfortunately, I'm not sure if there's a particularly "clean" way to solve this. I think if send is a crate-level feature, then you could do:

#[cfg_attr(feature = "send", async_trait)]
#[cfg_attr(not(feature = "send")), async_trait(?Send))]
trait BlockDevice { ... }

i.e. BlockDevice is only Send if the corresponding feature is active. This does make it a workspace-wide control, but it's the only non-insane option I can think of. EDIT: well turns out you need to copy the entire BlockDevice definition, with one behind cfg(feature = "send") that's trait BlockDevice: Send, didn't notice before because rust-analyzer wasn't communicating the errors to me 😅

@oll3
Copy link
Owner

oll3 commented Jan 23, 2022

Hm, I see your point. And I'm not really sure what a good solution is either.

Using a feature flag and possibly duplicating code just for avoiding Send feels a bit disturbing for such a small thing. Have you come across any other crate which uses feature flags to solve this? Otherwise I think I would rather prefer if BlockDevice was always just bound to Send.

Maybe @joshtriplett has some input on the matter since he proposed to add the ?Send to async_trait in #4 ? 🙂

@joshtriplett
Copy link
Contributor

@oll3 Unfortunately, I have the opposite use case: my implementation of BlockDevice isn't able to be Send. Changing BlockDevice to be Send would mean I can't use nbd-async.

Rather than a feature flag, I think it would make sense to have two types, one that implements Send and one that doesn't; that shouldn't actually duplicate the NBD protocol implementation, just the trait.

@oll3
Copy link
Owner

oll3 commented Jan 23, 2022

@joshtriplett I see. Then a separate trait which is Send might make sense. And I guess we might need a separate function for serve_nbd too.

@refi64 anything oppose this, would you mind creating a PR? 🙂

@joshtriplett
Copy link
Contributor

joshtriplett commented Jan 25, 2022

@oll3 FWIW, this issue (having to duplicate things for the Send and non-Send worlds) is one of many bits of friction that I think folks in the Async Foundations WG are trying to come up with solutions for.

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

No branches or pull requests

3 participants