-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add read_async to Resource
#5088
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
base: main
Are you sure you want to change the base?
Conversation
packages/hooks/src/use_resource.rs
Outdated
| drop(maybe_drop); | ||
| let _: () = (*self).await; | ||
| // `.read()` should have panicked if not in the correct scope as well | ||
| unreachable!("Future should cancel when ready"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API could be called inside of an event handler or spawn which won't reset the async block after the resource changes. How does this API behave in that use case:
button {
onclick: |_| async move { println!("resolved resource value is {:?}", resource.read_async(()) }
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested it quickly, If the resource is not ready, it hangs. I’ll look into a solution soon, but I can’t work on it today. Do you have any ideas how to solve this in the meantime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there was an issue with the original Future implementation for resource. No futures ever resolved if not already complete when first polled. I fixed this and added a way for Resource futures to work here. I removed the accepting other guards to drop in the api too because given these futures won't be canceled, the behavior won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are just missing a call to use_waker().wake() when the value is resolved? I would prefer to keep using the waker hook so we can maintain one version of that pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do call .wake() https://github.com/DioxusLabs/dioxus/pull/5088/files#diff-cffff841d5cfd91aa084e148058c59051af7956b3269eb35ac0db3811ac63c75L67 . I took a look and re-verified the issue exists.
The issue is self.waker.clone().poll_unpin(cx) will always return Pending (If the resource is already resolved we don't await/poll so this is never hit and that is why this case does not hang).
When .wake() is called it wakes up all the futures. The futures then re-poll and since .wake() replaces the internal waker channel, the new channel returns Pending again. Thus, I believe it was wrong for UseWaker to implement Future in its current form in the first place. I removed the implementation. It was only used in the original Future implementation for Resource so removing now has no effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some time today so I optimized the HashMap away with a versioned slab. It should surely be more efficient than the original UseWaker/SharedFuture Future implementation now too, since that used a lot of clones, channels, Mutexes, Arcs, and even Slab internally.
There currently is no ergonomic way to
awaitResources in an async closure. This PR adds theread_asyncmethod toResource