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

fix: add support for exposing functions that return objects #2

Merged
merged 3 commits into from
Sep 15, 2021

Conversation

namuol
Copy link
Contributor

@namuol namuol commented Sep 15, 2021

I've been experimenting with this library and ran into a problem which is best illustrated with this simple test:

test("expose function that returns object", async () => {
  const vm = (await getQuickJS()).createVm();
  const arena = new Arena(vm);

  arena.expose({
    makeObject: () => {
      return {
        myFavoriteNumber: 42,
      };
    },
  });

  expect(arena.evalCode(`makeObject().myFavoriteNumber`)).toBe(42);

  arena.dispose();
  vm.dispose();
});

While attempting to call evalCode, a Lifetime not alive error is thrown. I traced the issue to an instance where VMMap::_mapSet was attempting to be called in the VM but was not alive.

I believe the _mapSet instance was no longer alive because the original VMMap used to marshal the return value was created ephemerally here:

const map = new VMMap(this.vm);
map.merge(this._map);

Then, it is merged with the Arena::map instance again before it is disposed via .dispose(), here:

this._map.merge(map);
map.clear();
map.dispose();

Then, some time after the ephemeral map is disposed of, a callback passed to .consume must be executed and at this point _mapSet has already been disposed:

this.vm.newNumber(counter).consume((c) => {
if (!this._mapSet.alive) {
throw new Error("VMMap::set: `this._mapSet` is not alive!");
}
this._call(
this._mapSet,
undefined,
handle,
c,
handle2 ?? this.vm.undefined
);
});
return true;
}

To get around this, I'm avoiding the creation of the ephemeral map entirely, and instead I'm pointing directly to the Arena::map instance.

This fixed the issue and didn't appear to cause any new test failures, but I'm not certain this change is actually safe and would love to know what you think.

Thank you for your help, and for sharing your hard work - your library has been a pleasure to work with. 🙇‍♂️

@rot1024 rot1024 added the bug Something isn't working label Sep 15, 2021
@rot1024 rot1024 changed the title Add support for exposing functions that return objects fix: add support for exposing functions that return objects Sep 15, 2021
@rot1024 rot1024 merged commit c02505f into reearth:main Sep 15, 2021
@rot1024
Copy link
Member

rot1024 commented Sep 15, 2021

Thanks for your great PR. I just discovered this bug and was about to find out what caused it, but your PR saved me a lot of work! I will release a new version of quickjs-emscripten-sync as soon as possible!

@rot1024
Copy link
Member

rot1024 commented Sep 15, 2021

v1.2.0 has been released!

@namuol
Copy link
Contributor Author

namuol commented Sep 15, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants