search

jonhoo / async-bincode

Asynchronous access to a bincode-encoded item stream.
Apache License 2.0
68 stars 9 forks source link

Support both tokio and futures, via Cargo features #5

Closed joshtriplett closed 2 years ago

joshtriplett commented 2 years ago

Make tokio dependency optional via a "tokio" feature, and add support for futures via a "futures" feature.

This attempts to duplicate as little code as possible.

As implemented, exactly one of "futures" and "tokio" must be enabled; enabling both produces an error due to conflicting implementations of the same traits. Supporting both simultaneously would require substantially more duplication, and two distinct families of types.

jonhoo commented 2 years ago

Thanks for digging into this!

I think mutually exclusive features is a non-starter, unfortunately, since it's too likely to cause unresolveable issues for users just by virtue of how Cargo auto-merges features across a crate's dependency closure. It sucks that it only happens because the same Stream trait is used in both cases. That said, it should be possible to just have one impl, and then for the client to use an adapter type just for the R in the non-tokio case that adapts from impls of futures-io::AsyncRead to tokio::io::AsyncRead. It's not quite as ergonomic, but at least it makes the rest of the change not require the features to be mutually exclusive.

Barring that, I think the alternative is to split the implementation up further into multiple types. And maybe macros can help us make it so that little code is actually manually repeated. If we can't easily come up with such a macro, I wonder at that point if it even makes sense for those two impls to share a crate in the first place.

A standardized AsyncRead/AsyncWrite really can't come soon enough :sweat_smile:

joshtriplett commented 2 years ago

@jonhoo Adapting from one read trait to the other would require a dependency on both, rather than being able to feature-flag away the other dependency. However, if you don't mind adapter types, I do have an idea that'll allow both impls to coexist.

jonhoo commented 2 years ago

I'm not too concerned with the futures feature also pulling in tokio, as long as it does so with basically no features enabled, since then very little is compiled anyway.

joshtriplett commented 2 years ago

@jonhoo I am. I think I can make the two features completely independent without too much difficulty, using a wrapper type for the stream impl.

jonhoo commented 2 years ago

Released as 0.6.2 :tada: