Skip to content

POC on non static callback in TransactionBuilder::run #7

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

Conversation

touilleMan
Copy link
Contributor

@touilleMan touilleMan commented Mar 16, 2025

Here is a POC to showcase what I was thinking of in #5 (comment)

I guess in practice this unsafe stuff should be merged in the unsafe_jar internals, and may replace the need for Pin::new_unchecked.

@touilleMan touilleMan force-pushed the non-static-transaction-callback branch from 38f1341 to 3489baf Compare March 16, 2025 09:31
@Ekleog
Copy link
Owner

Ekleog commented Mar 16, 2025

Hmmm... so your code seems to make sense when written this way, but it assumes that unsafe_jar::run does not actually store the callback for longer than the given lifetime. Which could be wrong, eg. if the callback is never actually called and instead just dropped after the end of the lifetime?

I'd need to see how it integrates with unsafe_jar exactly to be sure, but IIRC my worries about this were on the front of when are destructors called.

Does that make sense?

@Ekleog
Copy link
Owner

Ekleog commented Mar 16, 2025

Also I just checked unsafe_jar, and there's not only Closure::once that could be problematic: there's also the current need for an Rc; so a solution that gets rid of 'static would also need to somehow not require that Rc.

Maybe an option would be to have a proper struct that could avoid the need for the Rc. But then we'd still need that struct to not contain a reference, because AFAICT js-bound structs cannot hold lifetimes.

Do you see any way to get this all to work?

BTW I think I just found a way to get rid of the unsafe keyword in unsafe_jar.rs: by having it be Rc<RefCell<Pin<Box<dyn Future>>>> instead of just Rc<RefCell<dyn Future>>. I'll see what I can do when I get time to tackle this!

@Ekleog
Copy link
Owner

Ekleog commented Mar 16, 2025

(Did manage to remove the unsafe with the last commit I just pushed 🎉 I don't know why I hadn't done that when originally developing indexed-db, probably I had too much on my mind then to notice this option)

@Ekleog
Copy link
Owner

Ekleog commented Mar 16, 2025

Ok, I opened #9 with a solution that I'm reasonably confident should not break everything. WDYT about it?

Unfortunately the solution wouldn't extend to the version-upgrade transaction, because said transaction is not run from an async block but from the sync closure.

@Ekleog
Copy link
Owner

Ekleog commented Mar 18, 2025

Closing in favor of #9/#12

@Ekleog Ekleog closed this Mar 18, 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