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

Make blocking APIs optionally async #100

Merged
merged 2 commits into from
Feb 12, 2025
Merged

Make blocking APIs optionally async #100

merged 2 commits into from
Feb 12, 2025

Conversation

kevinmehall
Copy link
Owner

@kevinmehall kevinmehall commented Dec 30, 2024

DeviceInfo::open, Device::from_fd, Device::set_configuration, Device::reset, Interface::set_alt_setting, Interface::clear_halt all perform IO but are currently blocking because the underlying OS APIs are blocking.

list_devices,list_buses, Device::claim_interface Device::detach_and_claim_interface theoretically don't perform IO, but are also included here because they need to be async on WebUSB.

The IoAction trait allows defering these actions to the thread pool from the blocking crate when used asynchronously with .await / IntoFuture, or directly runs the blocking syscall synchronously with a .wait() method.

Open question: Should the dependency on blocking be behind a cargo feature, and if unset, should IoAction not impl IntoFuture or should it return a dummy Future that runs synchronously?

@Yatekii
Copy link

Yatekii commented Jan 8, 2025

Personally I would completely forego blocking variants of those methods and make all of them async. The readme states Async-first and I would honor that claim. I do not see why we need a ::wait() when we can just .await the future. An async executor is required anyways for the actual read/write methods.
And we get to save one crate by default ;)

@kevinmehall
Copy link
Owner Author

The blocking dep (or tokio) is required to make these async either way -- it's not recommended to do something blocking from an executor thread, so this uses blocking to move the synchronous syscalls onto a threadpool, like tokio and async-fs do for synchronous filesystem APIs.

But then it seems kind of silly to do what is effectively block_on(unblock(|| x())) to make another thread do it and then wait for it, when you could just call x(), so that's what IoAction is.

@kevinmehall kevinmehall force-pushed the blocking-async branch 2 times, most recently from 3986e70 to 8532a37 Compare January 20, 2025 00:26
`DeviceInfo::open`, `Device::from_fd`, `Device::set_configuration`,
`Device::reset`, `Interface::set_alt_setting`, `Interface::clear_halt`
all perform IO but are currently blocking because the underlying OS APIs
are blocking.

`list_devices`,`list_buses`, `Device::claim_interface`
`Device::detach_and_claim_interface` theoretically don't perform IO, but
are also included here because they need to be async on WebUSB.

The `IoAction` trait allows defering these actions to the thread pool
from the `blocking` crate when used asynchronously with `.await` /
`IntoFuture`, or directly runs the blocking syscall synchronously with a
`.wait()` method.
@Yatekii
Copy link

Yatekii commented Jan 20, 2025

Fair point :) That's actually a really nice way to do this :)

I think we can merge this as is :)

@Yatekii
Copy link

Yatekii commented Feb 11, 2025

How are we looking on this? :) I would love to round up my WebUSB backend PR :)

@kevinmehall kevinmehall marked this pull request as ready for review February 12, 2025 01:50
@kevinmehall
Copy link
Owner Author

Renamed it, but I think it's ready.

@kevinmehall kevinmehall merged commit 14a8f17 into main Feb 12, 2025
8 checks passed
@kevinmehall kevinmehall deleted the blocking-async branch February 12, 2025 01:53
tuna-f1sh added a commit to tuna-f1sh/cyme that referenced this pull request Feb 12, 2025
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

Successfully merging this pull request may close these issues.

2 participants