Allow use in a web worker

[Fedeparma74]

This allows the use of the IndexedDB in a web worker scope, where the API is also available.

[Ekleog]

LGTM!

Just, do you think you could add a test, that'd spin up a web worker to validate that at least one basic write-then-read transaction works fine? :)

[Fedeparma74]

I already tried but unfortunately I wasn't able to get the IndexedDB to work (even in the main thread). To use web workers the SharedArrayBuffer should be enabled, and in Rust the way to do so is to recompile the std library with some additional features for the wasm32-unknown-unknown target. I used this config.toml:

[build]
target = "wasm32-unknown-unknown"
rustflags = ["-C", "target-feature=+atomics,+bulk-memory,+mutable-globals"]

[unstable]
build-std = ['std', 'panic_abort']

Just by enabling these features the current tests will fail :(

[Ekleog]

Hmm… So I can reproduce your failure, which seems to happen with the below config.toml, and not if removing +atomics.

[target.wasm32-unknown-unknown]
runner = "wasm-bindgen-test-runner"

[build]
rustflags = ["-C", "target-feature=+bulk-memory,+atomics"]

[unstable]
build-std = ['std', 'panic_abort']

I'm not entirely sure why that is; the error message (TransactionInactiveError) would usually mean that we returned to the event loop without having a request in flight. The only way I could see it happen would be, if sending a message on the mpsc::Sender did not result in the await-ing task immediately receiving the message. [Edit: this is exactly the issue]

This could theoretically happen, and is currently the only way to break the "transactions default to aborting" behavior. However, I would expect no async runtime to actually have that behavior, and with that behavior unfortunately no existing indexed-db library would work (AFAICT everyone relies on sending a message from the callback, except for indexed-db-futures, that just does the same but uses wakers manually instead of channels, and that'd have the same behavior).

Interestingly, I tried setting handlers for onabort and oncomplete, and none of them actually seemed to be called. But OTOH it happens on both firefox and chromium, so I'd be surprised if it were a browser bug. Still, that's a possibility: oncomplete and onabort not being called sounds weird.

Overall my guess would be, it's probably part of the reason why +atomics is currently unstable on the rustc side of things. I tried running indexed_db_futures tests with the same config.toml changes, and it failed too. (indexed_db_futures is the crate with the architecture as remote as possible from this crate).

Still seems weird to me, so if you have any other idea I'm willing to take it! And if not, I'm still good with landing this a simpler test that wouldn't rely on sharedarraybuffers & co, and just do things with basic message-passing :)

[Ekleog]

Actually I think I found it: my guess is it's this code of wasm-bindgen:
https://github.com/rustwasm/wasm-bindgen/blob/64e4a258a8be912702c54ae9c19e9cc720f6ca50/crates/futures/src/lib.rs#L56-L64

This switches the runtime to multithread, which probably means that transactions can auto-commit early. I'll try to figure out some way to avoid that auto-commit-early behavior :)

Edit: Confirmed, https://github.com/rustwasm/wasm-bindgen/blob/64e4a258a8be912702c54ae9c19e9cc720f6ca50/crates/futures/src/task/multithread.rs#L135-L158 creates a promise and that's not allowed during an indexeddb transaction

[Ekleog]

https://github.com/Ekleog/indexed-db/blob/wasm-bindgen-reproducer/tests/js.rs#L12-L53 showcases the difference in behavior between the singlethread and the multithread executor. I chatted with @daxpedda about that issue upstream, but I do think it's a bug in my IndexedDB crate (in which, granted, literally everyone else doing an async crate for indexeddb also fell), because it assumed a property that's not actually documented as holding, aka waking a task will make it run instantaneously.

I'll try to fix it on my end, and maybe the futures-based executor will also change to fix the other IndexedDB crates, and let you know when it's done :)

[Ekleog]

Ok, I have a fix up at #3 ; that said I'll wait until wasm-bindgen/wasm-bindgen#3798 sees a decision to land it, because it slightly changes the API and would thus require a breaking change.

This being said, feel free to use that branch for now to unblock yourself, and for this specific PR I can land it as soon as you have a very simple test for it, that doesn't have to use all the SharedArrayBuffer machinery :)

[Ekleog]

I looked a bit more at how to implement a test with a web worker, and it seems like it's quite a mess to do it directly with web-sys. Feel free to add a dev-dependency on gloo-worker to implement worker spawning :)

[Ekleog]

Welp, after a week no one had any ideas about how to fix this any other way, so I just released 0.4.0 that fixes the issues with the multi-threaded executor :)

[Fedeparma74]

Sorry for my late reply and contribution, I had a super busy week!

Thank you for the fix, I tested it and it worked. After spending some time trying to use message passing to test the crate in a worker scope I noticed that wasm-bindgen-test can be configured to run the tests in a web worker, so I added a test-worker feature for this. Let me know if you are fine with this 😄

[Ekleog]

Looks like a great way to test!

Just, I don't really feel good about adding a feature for that, given that indexed-db is published on crates.io. How about instead we:

• Move the contents of js.rs and js-panic.rs to some other folder, that is not the integration tests itself
• Make 4 files in the integration-tests folder. Each of them would first setup wasm-bindgen-test with either browser or worker, and then include the js.rs or js-panic.rs. The include could happen with either the include!() macro, or the #[path = "../test-utils/js.rs"] pub mod tests, if wasm-bindgen-test likes it enough

It should also help with running the tests, as this way a single cargo test would be enough to test all the situations, without requiring one cargo test and one cargo test --features test-worker.

WDYT? :)

[Ekleog]

Actually I just noticed this does not require include! or #[path] magic: serde-wasm-bindgen seems to have a similar working setup without all this complexity, with just a common module :)

[Fedeparma74]

That's great! It should be ready now :)

[Ekleog]

The new diff looks great to me! Just, you apparently didn't check the "allow maintainer edits" checkbox, so I can't resolve the conflicts for you without losing your authorship on the commits (I'll squash-merge once it's done). Can you just resolve them?

Then we can land, and I'll release 0.4.1 with the fixes! :)

[Ekleog]

I just noticed that you had quite a bit of luck with CI, because putting a panicking test in the same overarching test as any other test is UB, and currently fails if the test order is wrong 😅

Anyway, I fixed it with e31df68, and also made the code a bit more defensive with 85d1733 wrt. multi-threading.

I also just released 0.4.1, hopefully that version will work for you! :D