Skip to content

Use AsyncFnOnce instead of FnOnce -> RetFut #5

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

Closed
touilleMan opened this issue Mar 14, 2025 · 3 comments · Fixed by #6
Closed

Use AsyncFnOnce instead of FnOnce -> RetFut #5

touilleMan opened this issue Mar 14, 2025 · 3 comments · Fixed by #6

Comments

@touilleMan
Copy link
Contributor

Async closure has been stabilized in Rust 1.85 this brings a much better ergonomic when passing reference to a closure.

For instance currently this wrapper is not possible (but would be fine if TransactionBuilder::run would accept an async closure):

struct WrapperDatabase {
    pub async fn for_update<R, E>(
        &mut self,
        cb: impl AsyncFnOnce(TransactionWrapper),
    ) {
        self.conn.transaction(&["store"])
            .run(async move |transaction| {
                let wrapper = TransactionWrapper(transaction);
                cb(wrapper).await
                Ok(())
            })
            .await.unwrap();
    }
}

Rust compilation error:

the parameter type `impl AsyncFnOnce(TransactionWrapper)` must be valid for the static lifetime...
...so that the type `impl AsyncFnOnce(TransactionWrapper)` will meet its required lifetime bounds
@Ekleog
Copy link
Owner

Ekleog commented Mar 14, 2025

Oooh I hadn't followed the introduction of the AsyncFn family of functions! That's awesome, thank you for the pointer 😄

However, I must say that we'll probably still not be able to have it in this specific part of the code. The reason is that here, the 'static bound is actually on-purpose: in order to work around the indexed-db hitches that other binding crates expose, I'm doing some dark voodoo that guarantees that we never return to the javascript event loop without an ongoing request.

And I don't think I could write this safely without the 'static bound on the returned future 😭

This being said, if you have any ideas for how to handle that issue, I'd love to hear them! The 'static bound also annoys me quite a lot, as it requires Rc-ing everything and using things like Bytes instead of raw data/borrows

@touilleMan
Copy link
Contributor Author

I've opened a PR as a POC for supporting async closure

However, I must say that we'll probably still not be able to have it in this specific part of the code. The reason is that here, the 'static bound is actually on-purpose

This being said, if you have any ideas for how to handle that issue, I'd love to hear them! The 'static bound also annoys me quite a lot, as it requires Rc-ing everything and using things like Bytes instead of raw data/borrows

I haven't dig much in unsafe_jar, but my guess is 'static is required due to the use of wasm_bindingen::Closure::once.
wasm_bindingen::Closure::once itself requires 'static since there is no guarantee when the closure is actually called.

However in the case of IndexedDb I guess we have guarantees on this (typically the on_upgrade_needed closure in Factory::open shouldn't be called by the browser once the success/error callbacks has been called.
In such case we might be able to pin the async closure in Factory::open body, then type erase it to end up with some kind of &dyn Future that we could pass around as a raw pointer to whoever is supposed to poll it... this sound horribly unsafe though 😆

@Ekleog Ekleog closed this as completed in #6 Mar 15, 2025
@Ekleog
Copy link
Owner

Ekleog commented Mar 15, 2025

Hmm… I'd need to think quite a lot more about it before I consider adding a new unsafe block — I'm already quite a bit unhappy about having the one in Pin::new_unchecked I already have. This being said, the PR you send looks good to me — it doesn't touch the unsafe jar, so I'm happy about not having to care about that yet 😆 Thank you!

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 a pull request may close this issue.

2 participants