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

Add async functionality #8

Closed
wants to merge 1 commit into from

Conversation

danielstuart14
Copy link

@danielstuart14 danielstuart14 commented Aug 19, 2024

This new feature is gated behind the async feature flag, with the default behavior and syntax maintained.

It allows for asynchronously waiting for a pool slot to become available, making it easier to share constrained resources between multiple tasks.

It requires the AtomicWaker functionality from the embassy-sync crate, which in turn requires a critical section implementation.

Check the example for more details.

@danielstuart14 danielstuart14 changed the title Add async functionality & switch to portable-atomic Add async functionality Aug 19, 2024
@danielstuart14
Copy link
Author

I had to remove the change to portable-atomic as it was failing the CI/CD. I'll investigate it later and create a separate PR.

Let me know if this is something desired, as well as if the code is as you would like.
Thanks!

@danielstuart14 danielstuart14 marked this pull request as draft August 19, 2024 12:30
@danielstuart14 danielstuart14 force-pushed the master branch 2 times, most recently from b19b28b to de91803 Compare August 19, 2024 14:40
@Dirbaio
Copy link
Member

Dirbaio commented Aug 19, 2024

I think this should be a separate struct. Having an async Cargo feature means you either get async support on all pools or no pool. If you only need async support on some pools you're wasting RAM for wakers that won't be used in the ones where you don't use async.

Also in general I'm a bit skeptical of doing "wait until there's free memory", it makes it too easy to get deadlocks if for example the occupied memory is owned by the current task.

Also, I consider this crate "done", I'd prefer to not add more functionality. I'd suggest you work on this in a separate crate.

This new feature is gated behind the `async` feature flag.

It allows to asynchronously await for a pool slot to become available,
making it easier to share constrained resources between multiple tasks.

It requires the `AtomicWaker` functionality from the `embassy-sync`
crate, which in turn requires a critical section implementation.
@danielstuart14
Copy link
Author

I think this should be a separate struct. Having an async Cargo feature means you either get async support on all pools or no pool. If you only need async support on some pools you're wasting RAM for wakers that won't be used in the ones where you don't use async.

Also in general I'm a bit skeptical of doing "wait until there's free memory", it makes it too easy to get deadlocks if for example the occupied memory is owned by the current task.

Also, I consider this crate "done", I'd prefer to not add more functionality. I'd suggest you work on this in a separate crate.

That makes sense, I'll move this to a separate crate! Thanks!
I agree with you that blocking until memory is available isn't good, but with async, we can use a timeout and join it to be sure it won't deadlock, that's the reason I developed it.

Thanks!

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