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

feat: cleanup memory on rust panic #317

Merged
merged 3 commits into from
Jun 30, 2023

Conversation

mateuszjasiuk
Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk commented Jun 29, 2023

Context:

So I found a pretty shitty workaround. So it seems like the only way to gc the memory is to free all the references to the WebAssembly instance exports/properties. We can't do it manually by accessing memory or sth.
The problem tho is that because rust panics the js stops executing(just hangs there). Because of that extension service worker(which we use to run rust sdk code) can't get inactive(and drop the references to WebAssembly exports) 😕
So the shitty fix is to timeout calls to rust sdk, after certain amount of time. This way we can catch reject on timeout and handle it as a normal error. So we still have garbage in memory, but it will be freed after service worker goes inactive which is 30 seconds of no received messages. Unfortunately we can't stop the service worker manually, we can only deregister it, but then we can't register it again as this happens automatically on first extension load 😐

What've changed:

  • query functions are wrapped in timeout
  • added wasm build script for dev: wasm:build:dev
  • with wasm:build:dev we define panic_hook - added dev feature
  • changed build targets to es2015 to be able to extend generated Query class

We can do something similar for the Sdk class, it might be a bit tricky though as for example submit shielded transfer can take a while

@mateuszjasiuk mateuszjasiuk marked this pull request as ready for review June 29, 2023 13:46
@github-actions
Copy link

github-actions bot commented Jun 29, 2023

@github-actions github-actions bot temporarily deployed to pull request June 29, 2023 13:56 Inactive
(...args: U): Promise<T> => {
return new Promise(async (resolve, reject) => {
const t = setTimeout(() => {
reject(`Balance query timed out after ${timeout}s.`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could do something like promiseWithTimeout where we pass in the error message as an arg to display, as these are not all balance queries?

Copy link
Collaborator Author

@mateuszjasiuk mateuszjasiuk Jun 29, 2023

Choose a reason for hiding this comment

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

Ah yeah :) Focused too much on balances 👍

Copy link
Collaborator

@jurevans jurevans left a comment

Choose a reason for hiding this comment

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

I think this looks good! I just left a minor comment

@github-actions github-actions bot temporarily deployed to pull request June 29, 2023 16:29 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 30, 2023 08:13 Inactive
@mateuszjasiuk mateuszjasiuk merged commit 779bb6a into main Jun 30, 2023
5 checks passed
emccorson added a commit that referenced this pull request Jul 3, 2023
This fixes a bug introduced in #317 where `new Query` cannot be called
because Query was transpiled to ES5 and relies on the class it extends
to also be transpiled, but the parent class was generated from Rust
and not transpiled.

target JS version for namada-interface too.
emccorson added a commit that referenced this pull request Jul 3, 2023
This fixes a bug introduced in #317 where `new Query` cannot be called
because Query was transpiled to ES5 and relies on the class it extends
to also be transpiled, but the parent class was generated from Rust
and not transpiled.

PR #317 already fixed this for the extension; this commit just sets
the target JS version for namada-interface too.
@mateuszjasiuk mateuszjasiuk deleted the bug/cleanup-memory-on-extension-panics branch September 13, 2024 09:29
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