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

refactor: store bytes in vars instead of memory block index #71

Closed
wants to merge 2 commits into from

Conversation

zshipko
Copy link
Contributor

@zshipko zshipko commented Jun 17, 2024

Fixes an incompatibility identified here: extism/go-pdk#34

Storing the handle makes a lot of sense here, but the PDKs expect for the handles that are passed into var_set and returned from var_get to be transient.

Copy link
Contributor

@chrisdickinson chrisdickinson left a comment

Choose a reason for hiding this comment

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

Hm, I need to double-check the behavior here for workers. I think this would cause a desync between plugins run on a background worker and the host functions on the foreground, since the variable data would no longer be held in allocator memory. But! On the other hand, thinking through this, there might be a bug there already that this PR exposes 😅


this.#vars.set(name, oldIdx ?? newIdx);
return Block.indexToAddress(oldIdx ?? newIdx);
this.#vars.set(name, new PluginOutput(value.buffer));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there might be something tricky to navigate around here with background workers. I think this would fail in the following case:

createPlugin('path/to/some/plugin', {
  runInWorker: true,
  functions: {
    'a namespace': {
       async hostfunc(context) {
         context.getVariable("foo") // should return BAZ but returns null
         context.setVariable("foo", "bar")
       }
    }
  }
})

Where the pseudocode for the plugin would be something like:

Var.set("foo", "BAZ")
const {hostfunc} = Host.getFunctions();

func();
Var.get("foo") // should return "bar", but instead returns "BAZ"

because the worker and main thread only sync memory held in #blocks. I can see about writing this up as a test if that'd be helpful!

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 interesting, yeah I'm not 100% clear on some of the memory details - if you could add that test it would be super helpful!

In the meantime I will poke around and try to understand my changes a little better

@zshipko
Copy link
Contributor Author

zshipko commented Jun 18, 2024

It seems like it may already be a bug like you mentioned, I am using node to run test plugins that contain some code like this:

  plugin.setVar("test_var", "something");

  const c = plugin.getVar("test_var") catch unreachable orelse "";
  if (!std.mem.eql(u8, c, "something")) {
      unreachable;
  }

and it's hitting that unreachable block because setVar is calling free on the value before it returns. I also noticed that the rust-pdk leaks these values, which is why we may not have run into this with plugins written in Rust: extism/rust-pdk#57.

@zshipko zshipko changed the title refactor: store PluginOutput in vars instead of memory block index refactor: store bytes in vars instead of memory block index Jul 5, 2024
@chrisdickinson
Copy link
Contributor

I believe the js-pdk also leaks these values - it might be worth formalizing that behavior as "ownership of pointers passed to extism host functions are moved to the host"?

@zshipko
Copy link
Contributor Author

zshipko commented Jul 18, 2024

I believe the js-pdk also leaks these values - it might be worth formalizing that behavior as "ownership of pointers passed to extism host functions are moved to the host"?

Yeah I think that makes a lot of sense, I will investigate how Rust/Go handle pointers passed to the host and come up with some fixes.

@chrisdickinson
Copy link
Contributor

I think this is fixed in #80?

@zshipko
Copy link
Contributor Author

zshipko commented Jul 25, 2024

Yep, closing this!

@zshipko zshipko closed this Jul 25, 2024
zshipko added a commit to extism/extism that referenced this pull request Sep 23, 2024
…743)

This PR changes the host to take ownership of memory blocks passed into
Extism host functions, see:
extism/js-sdk#71 (comment)
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