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

Make idb_sys::Transaction Clone #16

Closed
wants to merge 1 commit into from

Conversation

Ekleog
Copy link

@Ekleog Ekleog commented Jan 13, 2024

This is more convenient for downstream users than having to cast to web_sys::IdbTransaction and back every time they want to clone.

@Ekleog
Copy link
Author

Ekleog commented Jan 13, 2024

Looks like CI is pretty broken, none of the changes seem related to my changes 😅

@devashishdxt
Copy link
Owner

Looks like CI is pretty broken, none of the changes seem related to my changes 😅

I don't think that the CI is broken. These changes are related to CI failure. When you don't store the closures in the IdbTransaction struct, they gets dropped after that function completes. It is necessary to keep those closures in memory until the transaction is dropped.

@@ -71,7 +68,6 @@ impl Transaction {
let closure = Closure::once(callback);
self.inner
.set_onabort(Some(closure.as_ref().unchecked_ref()));
self.abort_callback = Some(closure);
Copy link
Owner

Choose a reason for hiding this comment

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

Removing this will drop the closure from memory once this function completes. This may lead to undefined behaviour.

@Ekleog
Copy link
Author

Ekleog commented Jan 13, 2024

Good, point, thank you! Seeing the lints fail on code I didn't touch, I assumed that the index read was also a spurious failure, sorry 😅

I was actually coming back to this PR to close it myself after noticing my mistake. However, I'll argue that the current code does not actually work properly anyway: it means that one cannot call .on_success() from a .on_success() handler, which in turns means that the IndexedDb API is basically impossible to use properly for multi-requests transactions. That said I'm still trying to think of a good fix, and will come back to you if I find one :)

BTW, if you're up, can you have a look at #7 (comment)? It's not impossible I'll ragequit trying to use idb-sys manually tomorrow, in which case I'd likely start implementing something like what I described there, and it'd be great if I knew whether to implement it in my own code or in idb itself :)

@Ekleog
Copy link
Author

Ekleog commented Jan 13, 2024

Let's close this PR, I opened #17 to track the related issue

@Ekleog Ekleog closed this Jan 13, 2024
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