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

Correctly use context in plugin and provide alternative _WithContext methods #62

Merged
merged 7 commits into from Mar 12, 2024

Conversation

Marton6
Copy link
Contributor

@Marton6 Marton6 commented Mar 11, 2024

This PR ensures that the extism plugin struct does not store a context object in itself as this is not idiomatic Go and leads to unexpected behavior. Instead, a context object is passed to each plugin method call that runs a long running operation.

To not break the interface of the SDK, the existing SDK method signatures were preserved, but they were changed to use context.Background under the hood. Next to these, alternative _WithContext methods were added, which allow the caller to provide a custom context object.

…methods

This commit ensures that the extism plugin struct does
not store a context object in itself as this is not idiomatic
Go and leads to unexpected behavior. Instead, a context object
is passed to each plugin method call that runs a long running
operation.

To not break the interface of the SDK, the existing SDK method
signatures were preserved, but they were changed to use
context.Background under the hood. Next to these, alternative
_WithContext methods were added, which allow the caller to
provide a custom context object.
Copy link
Collaborator

@mhmd-azeez mhmd-azeez left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR, to understand the new behavior better, i think it's good to add these two test cases:

// make sure cancelling the context given to NewPlugin doesn't affect plugin calls
func TestContextCancel(t *testing.T) {
	manifest := manifest("sleep.wasm")
	manifest.Config["duration"] = "0" // sleep for 0 seconds

	ctx, cancel := context.WithCancel(context.Background())
	config := PluginConfig{
		ModuleConfig:  wazero.NewModuleConfig().WithSysWalltime(),
		EnableWasi:    true,
		RuntimeConfig: wazero.NewRuntimeConfig().WithCloseOnContextDone(true),
	}

	plugin, err := NewPlugin(ctx, manifest, config, []HostFunction{})

	if err != nil {
		t.Errorf("Could not create plugin: %v", err)
	}

	defer plugin.Close()
	cancel() // cancel the parent context

	exit, out, err := plugin.CallWithContext(context.Background(), "run_test", []byte{})

	if assertCall(t, err, exit) {
		assert.Equal(t, "slept for 0 seconds", string(out))
	}
}

// make sure we can still turn on experimental wazero features
func TestEnableExperimentalFeature(t *testing.T) {
	var buf bytes.Buffer

	// Set context to one that has an experimental listener
	ctx := context.WithValue(context.Background(), experimental.FunctionListenerFactoryKey{}, logging.NewLoggingListenerFactory(&buf))

	manifest := manifest("sleep.wasm")
	manifest.Config["duration"] = "0" // sleep for 0 seconds

	config := PluginConfig{
		ModuleConfig:  wazero.NewModuleConfig().WithSysWalltime(),
		EnableWasi:    true,
		RuntimeConfig: wazero.NewRuntimeConfig().WithCloseOnContextDone(true),
	}

	plugin, err := NewPlugin(ctx, manifest, config, []HostFunction{})

	if err != nil {
		t.Errorf("Could not create plugin: %v", err)
	}

	defer plugin.Close()

	var buf2 bytes.Buffer
	ctx = context.WithValue(context.Background(), experimental.FunctionListenerFactoryKey{}, logging.NewLoggingListenerFactory(&buf2))
	exit, out, err := plugin.CallWithContext(ctx, "run_test", []byte{})

	if assertCall(t, err, exit) {
		assert.Equal(t, "slept for 0 seconds", string(out))

		assert.NotEmpty(t, buf.String())
		assert.Empty(t, buf2.String())
	}
}

These two both pass currently, I think the behavior is acceptable, but I am curious about your opinions @nilslice @zshipko

runtime.go Outdated Show resolved Hide resolved
@Marton6
Copy link
Contributor Author

Marton6 commented Mar 11, 2024

Thank you very much for the PR, to understand the new behavior better, i think it's good to add these two test cases:

// make sure cancelling the context given to NewPlugin doesn't affect plugin calls
func TestContextCancel(t *testing.T) {
	manifest := manifest("sleep.wasm")
	manifest.Config["duration"] = "0" // sleep for 0 seconds

	ctx, cancel := context.WithCancel(context.Background())
	config := PluginConfig{
		ModuleConfig:  wazero.NewModuleConfig().WithSysWalltime(),
		EnableWasi:    true,
		RuntimeConfig: wazero.NewRuntimeConfig().WithCloseOnContextDone(true),
	}

	plugin, err := NewPlugin(ctx, manifest, config, []HostFunction{})

	if err != nil {
		t.Errorf("Could not create plugin: %v", err)
	}

	defer plugin.Close()
	cancel() // cancel the parent context

	exit, out, err := plugin.CallWithContext(context.Background(), "run_test", []byte{})

	if assertCall(t, err, exit) {
		assert.Equal(t, "slept for 0 seconds", string(out))
	}
}

// make sure we can still turn on experimental wazero features
func TestEnableExperimentalFeature(t *testing.T) {
	var buf bytes.Buffer

	// Set context to one that has an experimental listener
	ctx := context.WithValue(context.Background(), experimental.FunctionListenerFactoryKey{}, logging.NewLoggingListenerFactory(&buf))

	manifest := manifest("sleep.wasm")
	manifest.Config["duration"] = "0" // sleep for 0 seconds

	config := PluginConfig{
		ModuleConfig:  wazero.NewModuleConfig().WithSysWalltime(),
		EnableWasi:    true,
		RuntimeConfig: wazero.NewRuntimeConfig().WithCloseOnContextDone(true),
	}

	plugin, err := NewPlugin(ctx, manifest, config, []HostFunction{})

	if err != nil {
		t.Errorf("Could not create plugin: %v", err)
	}

	defer plugin.Close()

	var buf2 bytes.Buffer
	ctx = context.WithValue(context.Background(), experimental.FunctionListenerFactoryKey{}, logging.NewLoggingListenerFactory(&buf2))
	exit, out, err := plugin.CallWithContext(ctx, "run_test", []byte{})

	if assertCall(t, err, exit) {
		assert.Equal(t, "slept for 0 seconds", string(out))

		assert.NotEmpty(t, buf.String())
		assert.Empty(t, buf2.String())
	}
}

These two both pass currently, I think the behavior is acceptable, but I am curious about your opinions @nilslice @zshipko

Thanks for the tests. I've added them to the test suite!

Copy link
Contributor

@zshipko zshipko left a comment

Choose a reason for hiding this comment

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

Just a few small changes, but otherwise looks good!

extism.go Outdated Show resolved Hide resolved
extism.go Outdated Show resolved Hide resolved
extism.go Outdated Show resolved Hide resolved
extism.go Outdated Show resolved Hide resolved
Marton6 and others added 4 commits March 12, 2024 10:08
Co-authored-by: zach <zachshipko@gmail.com>
Co-authored-by: zach <zachshipko@gmail.com>
Co-authored-by: zach <zachshipko@gmail.com>
Co-authored-by: zach <zachshipko@gmail.com>
@nilslice nilslice merged commit 9101916 into extism:main Mar 12, 2024
3 checks passed
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.

None yet

4 participants