fix: fallback to manifest values if they're not specified in ExtismPluginOptions #73

mhmd-azeez · 2024-06-25T19:01:36Z

Fixes #58
Fixes #55

…uginOptions

mhmd-azeez · 2024-06-25T19:28:09Z

@chrisdickinson i am on the fence about whether we should ~~contact~~ concat the fields, or only use manifest when nothing is specified in ExtismPluginOptions

chrisdickinson

Thanks for taking a look at this! This PR jogs my memory - I think I was also on the fence about whether to allow the manifest to control allowedHosts/allowedPaths in the absence of explicit configuration from the caller. Since the manifest can be loaded via URL it strikes me that the manifest author and the sdk integrator might not be the same party.

At the risk of complicating things, I'd almost want to have an explicit createPlugin-only option like allowManifestConfig to determine what options the manifest is allowed to control.

chrisdickinson · 2024-06-25T19:40:52Z

src/mod.ts

@@ -91,7 +99,7 @@ export async function createPlugin(
 );
 }

- const [names, moduleData] = await _toWasmModuleData(await Promise.resolve(manifest), opts.fetch ?? fetch);
+ const [names, moduleData] = await _toWasmModuleData(m, opts.fetch ?? fetch);


In order to support manifest options when the manifest is provided via fetch, we might rework _toWasmModuleData to return a third item, manifestOpts, then perform this call before normalizing the options in this function.

intoManifest is the chokepoint at which all the disparate input types are unified into something "manifest-like", so we could extract manifest-provided options here and then use them in createPlugin to finish normalizing the input options.

is this what you meant?

js-sdk/src/manifest.ts

Line 97 in d3e0dbc

const manifestOpts : ManifestOptions = {

Yep! That's perfect, thank you!

mhmd-azeez · 2024-06-26T05:26:43Z

At the risk of complicating things, I'd almost want to have an explicit createPlugin-only option like allowManifestConfig to determine what options the manifest is allowed to control.

I am hesitant about this because this behavior is different from what the other SDKs are doing. Curious what @bhelx thinks

mhmd-azeez · 2024-06-26T06:40:33Z

Seems like manifest is missing a few options, I will try to add them all in this PR

chrisdickinson · 2024-06-26T16:20:44Z

src/utils.ts

@@ -0,0 +1,15 @@
+export function withTimeout<T>(promise: Promise<T>, timeoutMs?: number | undefined): Promise<T> {


To double-check semantics of the other sdks - in other sdks, it looks like if we cancel an operation due to a timeout, we expect the operation to halt (using increment_epoch).

If I understand this correctly, this is kind of a tricky feature to implement in JS. We can support this for runInWorker: true plugins – we can kill the worker thread on timeout – but I don't think we have a cancelable promise otherwise. This would be user-visible if the plugin has access to host functions.

Great points, while I was able to implement it in the background plugin, the foreground plugin doesn't seem to work:
43e75ac

If I can't find a solution, I might ignore timeout for now and create a separate issue for it

mhmd-azeez · 2024-06-30T15:09:33Z

@chrisdickinson i have added checks for cancellation before all of the extism functions + host functions. So the function call not being cancelled shouldn't be user visible. The only issue is timeouts don't seem to work while a wasm function is being executed. So ForegroundPlugin can't set cancelled to true. Any ideas?

Otherwise, we can just throw an exception when a timeout is specified for runInWorker: false

chrisdickinson · 2024-07-01T06:22:54Z

@chrisdickinson i have added checks for cancellation before all of the extism functions + host functions. So the function call not being cancelled shouldn't be user visible. The only issue is timeouts don't seem to work while a wasm function is being executed. So ForegroundPlugin can't set cancelled to true. Any ideas?

Ah yeah – unfortunately, since JS is single-threaded & Wasm runs on the main thread, control is never yielded back to the event loop to trigger the timer. I think it's probably best to make timers a runInWorker-only feature. If we do that, I think we might be able to remove the cancellation checks on CallContext and replace them with a worker.terminate() call (& some code to "restart" the worker from src/background-worker.ts.)

mhmd-azeez · 2024-07-02T08:40:31Z

@chrisdickinson thanks for your suggestion, I think it's much more robust now. Please take a look and lt me know if you have any more feedback

mhmd-azeez · 2024-07-02T10:28:09Z

for some reason, the tests fail in bun

mhmd-azeez · 2024-07-02T11:55:22Z

okay, not sure why only Bun is getting stuck. This is the minimal repro:
b6300e4

@chrisdickinson any ideas?

bhelx · 2024-07-02T14:05:29Z

Thanks for taking a look at this! This PR jogs my memory - I think I was also on the fence about whether to allow the manifest to control allowedHosts/allowedPaths in the absence of explicit configuration from the caller. Since the manifest can be loaded via URL it strikes me that the manifest author and the sdk integrator might not be the same party.
At the risk of complicating things, I'd almost want to have an explicit createPlugin-only option like allowManifestConfig to determine what options the manifest is allowed to control.

@chrisdickinson I think it's okay that manifest can be a url, but we should just draw bright lines around it. manifest wasn't ever really meant to be something a plug-in dev could author. We have had discussions about having some kind of separate "manifest override" kind of object that would give the host the controls to allow a guest to override specific parts of the manifest. But I think that is a bigger extism wide thing we need to propose and discuss.

chrisdickinson · 2024-07-02T16:34:04Z

any ideas?

Nothing too concrete. When I've run into Bun hangs and crashes before, it's been an outcome of their focus on performance. That tends to lead to bugs like subtle timing changes – so my first guess would be that they're doing something with the worker that causes it to spin up very quickly the second time its spawned, possibly before we've attached a message listener in the host. We could test this hypothesis by adding some logs to the host to see if or where the createWorker call is hanging – then, if it's hanging while waiting on either of these functions, we might add a setTimeout delay of 1ms or so to the worker before sending the initialized message.

The other thing that strikes me is that we null out this.#request here. That might be enough for the promise to get GC'd in other runtimes, but it could be that Bun's hanging on to it. It might be best to reject that promise by calling if (this.#request) this.#request[1](new Error('timed out')).

mhmd-azeez · 2024-07-02T16:55:03Z

Ah, it's actually node getting stuck at the end

mhmd-azeez · 2024-07-02T16:55:55Z

Sorry for doubting you Bun

bhelx · 2024-07-02T20:07:18Z

Sorry for doubting you Bun

check your nodejs bias at the door!

mhmd-azeez · 2024-07-04T09:07:33Z

Okay, as far as I can see, NodeJS can't terminate a worker that's executing a rogue wasm function. Here is a repro in our case:

  async call(funcName: string, input?: string | Uint8Array): Promise<PluginOutput | null> {
    const index = this.#context[STORE](input);

    try {
      const [errorIdx, outputIdx] = CAPABILITIES.supportsTimeouts ?
        await withTimeout(this.callBlock(funcName, index), this.opts.timeoutMs) :
        await this.callBlock(funcName, index);

      const shouldThrow = errorIdx !== null;
      const idx = errorIdx ?? outputIdx;

      if (idx === null) {
        return null;
      }

      const block = this.#context[GET_BLOCK](idx);

      if (block === null) {
        return null;
      }

      const buf = new PluginOutput(
        CAPABILITIES.allowSharedBufferCodec ? block.buffer : new Uint8Array(block.buffer).slice().buffer,
      );

      if (shouldThrow) {
        const msg = new TextDecoder().decode(buf);
        throw new Error(`Plugin-originated error: ${msg}`);
      }

      return buf;
    } catch (err) {
      if (err instanceof Error && err.message === 'Function call timed out') {
        console.log('Function call timed out, restarting worker')
        await this.restartWorker();
      }

      throw err;
    }
  }

test('plugin functions cant exceed specified timeout', async () => {
    let x = 0;

    try {
      const plugin = await createPlugin(
        { wasm: [{ url: 'https://localhost:8124/wasm/sleep.wasm' }], timeoutMs: 1 },
        {
          useWasi: true,
          functions: {
            "extism:host/user": {
              notify() {
                x++;
              },
              get_now_ms() {
                return BigInt(Date.now());
              }
            }
          },
          runInWorker: true
        });

      try {
        const [err, _] = await plugin.call('sleep', JSON.stringify({ duration_ms: 100 })).then(
          (data) => [null, data],
          (err) => [err, null],
        );

        assert(err)
        assert.equal(err.message, 'Function call timed out');
        await new Promise(resolve => setTimeout(resolve, 200));
        assert.equal(x, 0)

      } finally {
        await plugin.close();
      }
    }
    catch (err) {
      if (!CAPABILITIES.supportsTimeouts) {
        assert(err instanceof Error);
        assert.equal(err.message, 'Cannot set timeoutMs; current context does not support timeouts');
      }

      throw err;
    }
  });

async restartWorker() {
    await this.close();

    this.#context[RESET]();
    this.#request = null;

    this.worker = await createWorker(this.opts, this.names, this.modules, this.sharedData);
    this.worker.on('message', (ev) => this.#handleMessage(ev));
  }

async close(): Promise<void> {
    if (this.worker) {
      this.worker.terminate();
      this.worker = null as any;
    }
  }

Everything is executed (even the terminate call), but the process never exits.

I am going to skip timeout in this PR and create a separate issue for it

mhmd-azeez · 2024-07-04T11:01:07Z

This PR is ready for review

chrisdickinson

Whew, great work! I left some comments about places where we might still diverge from other extism host sdk behavior with regards to thrown errors. I also left an optional comment about a foible of mine.

If you'd like to merge from here I think this PR brings us a lot closer to the behavior of other sdks – we can make a ticket for handling error states more akin to how the rust-sdk handles it.

src/background-plugin.ts

src/call-context.ts

src/background-plugin.ts

chrisdickinson

LGTM! Great work!

mhmd-azeez added 4 commits June 25, 2024 22:00

fix: fallback to manifest values if they're not specified in ExtismPl…

c8070a4

…uginOptions

add tests

fb9b1a3

log manifest

7e73e7f

fix tests

d004368

mhmd-azeez marked this pull request as ready for review June 25, 2024 19:27

mhmd-azeez requested a review from chrisdickinson June 25, 2024 19:27

mhmd-azeez requested a review from bhelx June 25, 2024 19:28

chrisdickinson reviewed Jun 25, 2024

View reviewed changes

mhmd-azeez added 4 commits June 26, 2024 08:41

refactor

24c1f8a

fix tests

d3e0dbc

fix property names to match the json structure

5f658d4

add manifest timeout

99618e2

mhmd-azeez added 4 commits June 26, 2024 09:42

fix tests

4d2d57d

fix call timeout

70b1e71

fix tests

4c458e8

make manifest property names consistent with ExtismPluginOptions

4fe346f

chrisdickinson reviewed Jun 26, 2024

View reviewed changes

mhmd-azeez added 6 commits June 26, 2024 19:21

memory options

4568274

add tests for memory

0bff08b

add more tests

dd3908c

add timeout test

3248e19

add cancellation to call context

43e75ac

add guard before host functions

4f4edc8

mhmd-azeez added 2 commits July 1, 2024 11:39

timeout.wasm => sleep.wasm

7fdd7f6

use worker.terminate to cancel a call

aa3b507

mhmd-azeez added 2 commits July 2, 2024 11:50

refactor

9914611

fix build

c033ea6

mhmd-azeez added 4 commits July 2, 2024 13:43

revert background plugin cancellation work

355d1ad

refactor

4bf6cc9

bring back cancellation

c68125d

fix build

b6300e4

mhmd-azeez added 2 commits July 4, 2024 12:08

refactor

6404b64

remove timeout

5032877

chrisdickinson approved these changes Jul 4, 2024

View reviewed changes

src/background-plugin.ts Show resolved Hide resolved

src/background-plugin.ts Show resolved Hide resolved

src/background-plugin.ts Show resolved Hide resolved

src/call-context.ts Outdated Show resolved Hide resolved

src/background-plugin.ts Outdated Show resolved Hide resolved

mhmd-azeez added 7 commits July 7, 2024 10:39

apply chris' suggestions

2a8f747

use log_error instead of error_set

8cbfdd8

formatting

b868ec5

use release build

a780636

fix tests

9630a69

fix test

13fa089

pass worker into BackgroundPlugin's ctor

35495de

chrisdickinson approved these changes Jul 8, 2024

View reviewed changes

mhmd-azeez merged commit 389a732 into main Jul 9, 2024
4 checks passed

mhmd-azeez deleted the fix/manifest-options branch July 9, 2024 06:43

mhmd-azeez mentioned this pull request Jul 9, 2024

Support timeout in manifest #74

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fallback to manifest values if they're not specified in ExtismPluginOptions #73

fix: fallback to manifest values if they're not specified in ExtismPluginOptions #73

mhmd-azeez commented Jun 25, 2024 •

edited

Loading

mhmd-azeez commented Jun 25, 2024 •

edited

Loading

chrisdickinson left a comment

chrisdickinson Jun 25, 2024

mhmd-azeez Jun 26, 2024

chrisdickinson Jun 26, 2024

mhmd-azeez commented Jun 26, 2024

mhmd-azeez commented Jun 26, 2024

chrisdickinson Jun 26, 2024

mhmd-azeez Jun 27, 2024

mhmd-azeez commented Jun 30, 2024 •

edited

Loading

chrisdickinson commented Jul 1, 2024

mhmd-azeez commented Jul 2, 2024

mhmd-azeez commented Jul 2, 2024

mhmd-azeez commented Jul 2, 2024

bhelx commented Jul 2, 2024

chrisdickinson commented Jul 2, 2024

mhmd-azeez commented Jul 2, 2024

mhmd-azeez commented Jul 2, 2024

bhelx commented Jul 2, 2024

mhmd-azeez commented Jul 4, 2024

mhmd-azeez commented Jul 4, 2024

chrisdickinson left a comment

chrisdickinson left a comment

		@@ -0,0 +1,15 @@
		export function withTimeout<T>(promise: Promise<T>, timeoutMs?: number \| undefined): Promise<T> {

fix: fallback to manifest values if they're not specified in ExtismPluginOptions #73

fix: fallback to manifest values if they're not specified in ExtismPluginOptions #73

Conversation

mhmd-azeez commented Jun 25, 2024 • edited Loading

mhmd-azeez commented Jun 25, 2024 • edited Loading

chrisdickinson left a comment

Choose a reason for hiding this comment

chrisdickinson Jun 25, 2024

Choose a reason for hiding this comment

mhmd-azeez Jun 26, 2024

Choose a reason for hiding this comment

chrisdickinson Jun 26, 2024

Choose a reason for hiding this comment

mhmd-azeez commented Jun 26, 2024

mhmd-azeez commented Jun 26, 2024

chrisdickinson Jun 26, 2024

Choose a reason for hiding this comment

mhmd-azeez Jun 27, 2024

Choose a reason for hiding this comment

mhmd-azeez commented Jun 30, 2024 • edited Loading

chrisdickinson commented Jul 1, 2024

mhmd-azeez commented Jul 2, 2024

mhmd-azeez commented Jul 2, 2024

mhmd-azeez commented Jul 2, 2024

bhelx commented Jul 2, 2024

chrisdickinson commented Jul 2, 2024

mhmd-azeez commented Jul 2, 2024

mhmd-azeez commented Jul 2, 2024

bhelx commented Jul 2, 2024

mhmd-azeez commented Jul 4, 2024

mhmd-azeez commented Jul 4, 2024

chrisdickinson left a comment

Choose a reason for hiding this comment

chrisdickinson left a comment

Choose a reason for hiding this comment

mhmd-azeez commented Jun 25, 2024 •

edited

Loading

mhmd-azeez commented Jun 25, 2024 •

edited

Loading

mhmd-azeez commented Jun 30, 2024 •

edited

Loading