Skip to content

POC Non static future in unsafe jar (with Factory::open support but also more complexity...) #11

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

Conversation

touilleMan
Copy link
Contributor

No description provided.

Copy link
Owner

@Ekleog Ekleog left a comment

Choose a reason for hiding this comment

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

I already prefer this one to #10, because it seems better-encapsulated to me :)

I'll finish what I'm thinking of with #9 (just had an idea to get everything put together I think), let a bit of time pass and come back to the question later, I think :)

// cannot outlive it.
// This also means `DeferredRunning.future_done_rx` shares the same lifetime than the
// future, and we systematically check this object is still alive (using
// `tx.is_cancelled()`) before polling the future.
Copy link
Owner

Choose a reason for hiding this comment

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

Actually… this is not safe, because DeferredRunning could be leaked. So this fails due to the leakpocalypse 😭

Too bad, I was thinking that your approach was even cleaner than mine, based on the Rc's refcount!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah you're right, if we leak DeferredRunning then the pinned future may get deleted while the tx.is_cancelled() still returns false 😭

Since this structure is purely internal to indexed-db there is no risk of crazy misuse by 3rd party actors, but even without I can understand you don't want to introduce such footgun ;-)

@touilleMan
Copy link
Contributor Author

I already prefer this one to #10, because it seems better-encapsulated to me :)

Be careful though ! PR #10 is passing the tests fine, but this one has some hiccup with the panic tests

     Running tests/browser_panic.rs (target/wasm32-unknown-unknown/debug/deps/browser_panic-37a216998f32c868.wasm)
Running headless tests in Firefox on `http://127.0.0.1:44089/`
Try find `webdriver.json` for configure browser's capabilities:
Not found
Failed to detect test as having been run. It might have timed out.
output div contained:
    running 2 tests
    test common_panic::await_in_versionchange_panics ... ok

driver status: signal: 9 (SIGKILL)                
driver stdout:
    1742143320610	geckodriver	INFO	Listening on 127.0.0.1:44089
    1742143320713	mozrunner::runner	INFO	Running command: MOZ_CRASHREPORTER="1" MOZ_CRASHREPORTER_NO_REPORT="1" MOZ_CRASHREPORTER_SHUTDOWN="1" "/usr/bin/firefox" "--marionette" "-headless" "-no-remote" "-profile" "/tmp/rust_mozprofileZR8f9T"
    console.warn: services.settings: Ignoring preference override of remote settings server
    console.warn: services.settings: Allow by setting MOZ_REMOTE_SETTINGS_DEVTOOLS=1 in the environment
    1742143320911	Marionette	INFO	Marionette enabled
    1742143321272	Marionette	INFO	Listening on port 42505
    Read port: 42505
    [GFX1-]: RenderCompositorSWGL failed mapping default framebuffer, no dt
    console.error: ({})
    1742143342366	Marionette	INFO	Stopped listening on port 42505

driver stderr:
    *** You are running in headless mode.
    JavaScript error: http://127.0.0.1:40125/wasm-bindgen-test_bg.wasm, line 562373: RuntimeError: unreachable executed
    JavaScript error: http://127.0.0.1:40125/wasm-bindgen-test_bg.wasm, line 462034: RuntimeError: index out of bounds
    JavaScript error: http://127.0.0.1:40125/wasm-bindgen-test, line 871: Error: closure invoked recursively or after being dropped

I was debugging it when I noticed your messages, so I'm going to leave it as is (just tell me if you think this approach might be worth putting more effort in it ^^).

@Ekleog
Copy link
Owner

Ekleog commented Mar 16, 2025

Got it, thank you! And you're right I only grepped for the unsafe block without further review for now, just to get an idea of what was guaranteeing safety 😅

Unfortunately due to the leakpocalypse issue I don't think this option is viable 😢 I'm trying to get the rc-refcount solution (that works the other way around, by ensuring all references have been dropped, so leaking would just result in an abort) to work with the version upgrade transaction, hope I'll get something working!

And thank you for all your efforts anyway, regardless of which PR ends up landing 💜

@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