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

Feat/async support #95

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

xgroleau
Copy link
Contributor

@xgroleau xgroleau commented Oct 28, 2022

Initial draft for async support. Just want your input before continuing/cleaning up. My experience with async is pretty limited so feedback and comments are more than welcomed!

I diverted a bit from the previous work API since I wanted to keep the flexibility.
This async api is as close as possible to the sync one.

Status:

  • Basic implementation
  • Basic tests
  • Add Frame flavor implementation
  • Cancellation tests
  • Test on MSRV, gate behind feature if necessary
  • Complete API documentation

@xgroleau xgroleau marked this pull request as ready for review November 1, 2022 14:35
Copy link

@jeff-hiner jeff-hiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really useful, thanks! I'm going to try it in some network code and see how it works out.


#[test]
fn frame_wrong_size() {
block_on(async {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like some of these tests are invoking the sync and not async versions (grant instead of grant_async). Is that intended?


/// Read waker for async support
/// Woken up when a commit is done
read_waker: WakerStorage,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think gating these under a feature is the best idea here, especially considering how this is intended for embedded. That way anyone not using async doesn't need these extra fields.

@xgroleau
Copy link
Contributor Author

xgroleau commented Jan 9, 2023

This looks really useful, thanks! I'm going to try it in some network code and see how it works out.

Thanks @jeff-hiner! I continued developing the fork a bit since the PR, there are probably some fixes missing in this PR versus the fork. You can check it out here. Also note that the fork abstracted on the source of the storage since I wanted to provide the buffer myself.

I spoke to James some time ago and he won't merge the PR for now but we left it as reference for other users.

Since I am using it for internal project, I did not take the time to publish as a separate crate, but if there is some traction, I could publish a crate of the fork until the upstream gets async support if I have the time to update the doc.

@Altair-Bueno
Copy link

Just curious, could you elaborate on why you choose to leverage the waker? To my understanding, the executor will eventually poll the task anyway, so it shouldn't be a big issue. Specially given that the comparison of some atomic values is so fast. Using the waker leads to increased complexity on the codebase. Do you fear task starvation from the scheduler?

@xgroleau
Copy link
Contributor Author

xgroleau commented Jun 5, 2023

Just curious, could you elaborate on why you choose to leverage the waker? To my understanding, the executor will eventually poll the task anyway, so it shouldn't be a big issue. Specially given that the comparison of some atomic values is so fast. Using the waker leads to increased complexity on the codebase. Do you fear task starvation from the scheduler?

Sure! The thing is that most executor won't poll again unless the task is awoken, thus you need the waker. Without a waker, the executor would need to poll every task. So unless you use an executor that does a busy polling loop, the executor will not poll the task again like you mentionned.

For example, embassy puts the MCU in a low power state when all tasks are pending and wait for an interrupt. Needing to loop while polling would make this impossible.

You can learn more about the inner workings of async here

@jeff-hiner
Copy link

Just FYI, I've been using a fork of some of @xgroleau 's work for implementing a completion-based i/o model with async. I did run into some deadlock issues with the custom waker, so I patched in futures::task::AtomicWaker and replaced a bunch of NonNull and raw pointers with normal refs (which removed a bunch of unsafe). I don't know exactly what fixed the issue, and I need to break the fixes out into smaller PRs, but it's been running a while in production with no issues. Here's the branch if you want to experiment with it or incorporate any of the changes into your own fork.

https://github.com/jeff-hiner/bbqueue/tree/xgroleau-async

@Altair-Bueno
Copy link

The thing is that most executor won't poll again unless the task is awoken,

Hm i didn't knew that! Thanks.

And thanks Jeff too! I definitely will give it a try.

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.

3 participants