oll3/nbd-async

The result of serve_nbd is never Send

refi64 opened this issue ยท 4 comments

#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 commented

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 ? ๐Ÿ™‚

@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 commented

@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? ๐Ÿ™‚

@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.