added support for an async feature

[divinusdracodominus]

I added a feature switch to switch between std::os::unix::UnixDatagram and tokio::net::UnixDatagram this allows someone to use async/await (which is especially useful for AttachedClient as runtime cycles aren't consumed waiting for an event)

[spease]

Thanks for this! I’d do it a little differently though. I’d have a “sync” module for the synchronous implementation and the async implementation be the default. I believe this is how reqwest does it.

Though, that might be a breaking change for users, so it might need to be the opposite direction, but this runs afoul of “async” being a keyword. I don’t know if there are crates that use this approach or if there’s an opposite de facto standard to reqwest defaulting to async.

But anyway, I think this will go “with the grain” of features better because things usually assume that a feature is additive rather than replacing. In this way a dependency tree can use the same “wpactrl” for both crates using the sync and async api.

I’d probably also make the sync implementation just a wrapper around the async implementation; I don’t know if it’s as performant, it’s just easier to maintain since then you only have one set of logic, and I suspect any performance cost is negligible compared to I/O overhead.

Let me know what you think.

[apbr]

I’d probably also make the sync implementation just a wrapper around the async implementation;

Wouldn't this mean using the sync implementation of wpa-ctrl-rs would require tokio as dependency?
This could be quite annoying for sync users.

[spease]

I think you could use futures::executor::block_on. Or maybe you can call poll directly in a loop? I’d look at how other libraries are doing it, but I’m pretty sure there’s a solution without pulling in tokio.

[divinusdracodominus]

a problem I see with having a sync impl wrap an async version is that it would cause binary bloat as tokio is a pretty large dependency that some may not want in the dependency tree (if for example developing for embedded systems), tokio would be required as a dependency of wpa-ctrl-rs if sync wrapped async for the use of tokio::net::UnixDatagram.

when originally submitting this pole request I made the changes that'd be the fastest to implement without breaking anything as I didn't expect much of a response, and wanted async for my own project, but in light of this thread (and the interest it demonstrates) I'd be happy to implement a sync wrapper around the async default (which seems to be the best course of action)

as for the issue of breaking crates, sync could be enable as default, and the API for sync could be compatible with current implementation, just have current API (request, attach, detach) be wrappers around for example (query, bind, unbind)

[spease]

It's disappointing if it really is that much larger; it should only need the net feature of tokio, and the actual underlying system calls for the UDP socket shouldn't be much different than the sync version. The linker is also supposed to strip unused sections out. So, I'm a little skeptical, but I don't know well enough to rule it out as a concern without trial and error.

I'd appreciate it if you could give it a try, but just let me know.

[spease]

Can this be changed back?

[spease]

Where are these three dependencies used?

[spease]

Remove dead code

[spease]

There's actually still more implementation here than I was thinking. I'd just have sync::ClientAttached wrap async::ClientAttached, and calls generally just block_on their async equivalent rather than replicating the internal state. Ditto for Client.

[spease]

I think cb should be passed through. Although, IIRC, this may have just been to mirror the C API.

[spease]

Thanks!! I added some comments. Let me know if these make sense. It would also be helpful if you can squash the commits - it's a little difficult to tell what the end result is since there are two approaches and some of the changes are split between them.

Also, I don't think sync needs to be a feature, but I'm also not sure. If you're checking how much binary size increase there is with the changes, LMK, otherwise I plan to do so before I merge things in.

EDIT: Note that there have been PRs in the past to reduce the dependencies of the lib, so there at least are or were some users who do care about that to some degree. So now that I say that, maybe don't squash the changes yet, or at least only squash down to separate "async as feature" and "async/sync both in the library" changes.

[divinusdracodominus]

I'll make the suggested changes (having sync not maintain its own internal state) when I have more time, for now to answer your questions: https://github.com/divinusdracodominus/wpa-ctrl-rs/blob/dev/src/hostapd.rs is a module I created (mostly for my own use, as I don't think it'd necessarily be useful for everyone) a module that defines hostapd MIB variable structures such as 8021x for interop with postgres/serde/etc.

[spease]

Hey guys, sorry there's been no externally visible movement on this.

I had spent some significant time testing the PR and fixing it up a bit. The issue I got blocked on was the "who hosts the reactor issue" described in the recent "bane of my existence" post:

https://nullderef.com/blog/rust-async-sync/

However, I came back to this tonight and I'm realizing a further issue is that it also splits the ecosystem if someone wants to use this as the basis for additional changes (which I have some code to do, and @divinusdracodominus has https://github.com/divinusdracodominus/wpa-ctrl-rs/blob/dev/src/hostapd.rs).

You can always block_on to turn async into sync, and it looks like the post-strip release size only goes from 71k to 153k (so basically doubles). Or pre-strip 101k to 189k (iirc). Although even with default-features = false and keeping enabled feature/crates to a minimum, the number of crate dependencies balloons from 3 to 18, even though compiletime is still minimal.

Conversely, if the crate were sync-only, it would be entirely nominally incompatible with async code. While the basic stuff is probably not going to push past the limits that would break the executor with a unix socket syscall, anything that waits for wpa_supplicant to generate a certain reply that's based on WiFi activity probably would.

There are solutions to generate sync and async flavors of code (maybe_async and async-generic) but these would require downstream traits / types / crates to do the same as well. I'm not sure if these would handle doc comments and doctests either, so those would probably have to commit to one and/or have more complexity.

This seems to cut in favor of just supporting async, but this would be a breaking change for anyone currently using the crate.

Wondering what people's thoughts on this would be?