Alorel / rust-indexed-db

Future bindings for IndexedDB via web_sys
MIT License
46 stars 8 forks source link

indexed_db_futures correctness currently relies on invalid assumptions about the executor #33

Open Ekleog opened 9 months ago

Ekleog commented 9 months ago

Currently, indexed_db_futures relies on behavior of the futures executor that is not actually specified: it assumes that waking a task from an IndexedDB callback, and then returning from the IndexedDB callback, will result in the awoken task being executed before returning to the browser event loop.

This is not necessarily true, and in particular breaks with the multi-threaded wasm-bindgen executor. I opened an issue upstream, but unfortunately it seems hard to fix, and was closed as wontfix, because that specific behavior of the singlethread executor was never documented in the first place:

I'm opening this issue to let you know about this current limitation of indexed_db_futures. Please don't just comment on the upstream issue without carefully considering whether you're bringing something new to the table, as that would only be bad vibes for the wasm-bindgen maintainers, and they're doing an awesome job.

On my end I'm planning to fix this in my IndexedDB crate via this change that makes the transaction only ever execute from the callbacks. I verified and it works with the multi-threaded executor, but it also requires a slightly less convenient API. I'm hoping one of the places where I'm opening this issue will have an idea for how to handle this differently :)

Hope that helps!

ptdecker commented 4 months ago

bumping. I second this concern

Alorel commented 2 weeks ago

Hello, I love replying to things in a timely manner :melting_face: and first off, thank you for the time you've put into this - went though the issue on wasm-bindgen and there was a lot of digging involved!

I thought about this issue quite a bit while rewriting the library and ultimately decided to leave it and focus the library on a convenient to use API in a single-threaded environment - your crate already focuses on multi-threading and I couldn't find a way to make it work without making the same usability sacrifices; I don't think there's much point in reinventing the wheel in two crates and decided to take mine in a different direction.

I've added a section on the multi-threaded executor with a workaround that will suffice for some users and, for anyone wanting more, I just point them to indexed_db.

Ekleog commented 2 weeks ago

Thank you for your answer! And wish you all the best in the continuation of indexed_db_futures, then. Don't hesitate to hit me up if you ever have any new idea for another way to handle this issue!