propagate Go context into HostFunctionCallContext #203

pkedy · 2022-02-04T14:47:16Z

This PR adds an alternate store.CallFunctionContext(ctx context.Context, moduleName, funcName string, params ...uint64) func so the host can set values in a Go context that are accessible via the context.Value in *wasm.HostFunctionCallContext. This would allow custom data as well as distributed tracing values to flow through the call stack in a way that Go developers would expect.

See examples/host_func_test.go for usage

This is a simpler and more idiomatic alternative to #183 and #184.

/cc @codefromthecrypt

codefromthecrypt

I think we should rip the bandaid and make Call require a context param. breaking the api has little effect now, and we'll have to break it again later. Otherwise looks great!

wasm/engine.go

examples/host_func_test.go

codefromthecrypt · 2022-02-05T00:13:37Z

examples/host_func_test.go

@@ -37,8 +44,8 @@ func Test_hostFunc(t *testing.T) {
 		bufAddr := ret[0]

 		// Store the address info to the memory.
-		binary.LittleEndian.PutUint32(ctx.Memory.Buffer[retBufPtr:], uint32(bufAddr))
-		binary.LittleEndian.PutUint32(ctx.Memory.Buffer[retBufSize:], bufferSize)
+		ctx.Memory.PutUint32(retBufPtr, uint32(bufAddr))


not for this PR, but this reminds me we are overdue making things interfaces as direct access to buffer like this is easy to get wrong! #204

wasm/store.go

codefromthecrypt · 2022-02-05T00:30:03Z

wasm/store.go

@@ -314,6 +315,10 @@ func (s *Store) Instantiate(module *Module, name string) error {
 }

 func (s *Store) CallFunction(moduleName, funcName string, params ...uint64) (results []uint64, resultTypes []ValueType, err error) {


I think we can delete this option and then don't have to name it differently.

ex. dapr where there's no option except with context, it is simpler naming

https://github.com/dapr/go-sdk/blob/main/client/client.go

Since this is an early stage, we can break api now

Did you know that I work on Dapr or was this coincidence? 😀

I did notice 😎 so decided to speak Lingua Dapra

wasm/engine.go

codefromthecrypt · 2022-02-05T00:36:51Z

PS context propagation is absolutely important and best to decouple from things like lifecycle hooks. Later, we'll likely build on this with a begin/end hook design which can allow transparent metrics or tracing. This is bedrock for that also.

mathetake · 2022-02-05T00:56:27Z

Yeah I like this

pkedy · 2022-02-06T14:19:16Z

I think we should rip the bandaid and make Call require a context param. breaking the api has little effect now, and we'll have to break it again later.

@codefromthecrypt Totally agree! Changes made.

codefromthecrypt

one doc backfill suggestion, then a question of how or if we want to handle a nil context on the CallFunction API. Otherwise, LGTM

wasm/store.go

codefromthecrypt · 2022-02-06T23:04:41Z

wasm/store.go

 			return fmt.Errorf("calling start function failed: %v", err)
 		}
 	}
 	return nil
 }

-func (s *Store) CallFunction(moduleName, funcName string, params ...uint64) (results []uint64, resultTypes []ValueType, err error) {
+// Note: this API is unstable. See tetratelabs/wazero#170
+func (s *Store) CallFunction(ctx context.Context, moduleName, funcName string, params ...uint64) (results []uint64, resultTypes []ValueType, err error) {


Should we fail early, fail late or default any nil context here?

My gut says to set it to context.Background() so that host calls can assume a valid context is passed. That or fail/return an error.

ok let's go with context.Background, and update the docs to say that's what it defaults if nil. TestStore_CallFunction should verify this is never nil

codefromthecrypt · 2022-02-06T23:09:53Z

wasm/store.go

@@ -844,6 +847,7 @@ func DecodeBlockType(types []*TypeInstance, r io.Reader) (*FunctionType, uint64,

 // HostFunctionCallContext is the first argument of all host functions.
 type HostFunctionCallContext struct {
+	context.Context


I think we don't typically use struct embedding, so we should trailing comment why if we do. Alternatively, we can de-embed this. wdyt @pkedy @mathetake @nullpo-head?

Suggested change

context.Context

context.Context

yeah without concrete reason, we would like to avoid embedding

but this case seems reasonable to me so trailing comments would help

Embedding context I think is pretty common in web frameworks so you can pass it along. The standard http library does not so it might be a matter of preference. What are the reasons not to embed it? Just curious 😀

Clarifying the owner of fields/methods at callsite is one!

Nice. I have the change ready. Committing now.

I will say that it will be a little weird that Context() is a func and Memory is a field. But as you said, this will be reworked anyway.

Ack and I promise I'll do my best to unweird it soon

also PS I'm not doing this now because I don't want to drift your PR :D

wasm/store.go

codefromthecrypt · 2022-02-07T02:29:25Z

ps oops on arm drift

# github.com/tetratelabs/wazero/wasm/jit [github.com/tetratelabs/wazero/wasm/jit.test]
[14](https://github.com/tetratelabs/wazero/runs/5087432982?check_suite_focus=true#step:5:14)
Error: wasm/jit/jit_arm64_test.go:40:24: not enough arguments in call to engine.Call
[15](https://github.com/tetratelabs/wazero/runs/5087432982?check_suite_focus=true#step:5:15)
	have (*wasm.FunctionInstance)
[16](https://github.com/tetratelabs/wazero/runs/5087432982?check_suite_focus=true#step:5:16)
	want (context.Context, *wasm.FunctionInstance, ...uint64)

…alls Signed-off-by: Phil Kedy <phil.kedy@gmail.com>

…ion (breaking change)

…c instead of embedding it.

Signed-off-by: Phil Kedy <phil.kedy@gmail.com>

mathetake

excellent!

Signed-off-by: Phil Kedy <phil.kedy@gmail.com>

codefromthecrypt

@pkedy will merge when you say go. good work, here

pkedy · 2022-02-07T04:48:05Z

Good to go. Thanks!

codefromthecrypt · 2022-02-21T12:21:38Z

ps drift alert that this was renamed to ModuleContext. Also, if you are using something besides tinygo and/or need a WASI API not defined here, please pipe up in the issue #271

pkedy mentioned this pull request Feb 4, 2022

Custom data that is passed to host calls via HostFunctionCallContext #183

Closed

codefromthecrypt approved these changes Feb 5, 2022

View reviewed changes

codefromthecrypt changed the title ~~CallFunctionContext to pass a Go Context through to host calls~~ propagate Go context into HostFunctionCallContext Feb 5, 2022

pkedy force-pushed the call_function_context branch from e08a024 to e4c1736 Compare February 6, 2022 14:28

pkedy marked this pull request as ready for review February 6, 2022 14:29

codefromthecrypt approved these changes Feb 6, 2022

View reviewed changes

codefromthecrypt reviewed Feb 6, 2022

View reviewed changes

pkedy force-pushed the call_function_context branch from e4c1736 to f0c1643 Compare February 7, 2022 01:53

mathetake assigned codefromthecrypt Feb 7, 2022

pkedy mentioned this pull request Feb 7, 2022

Add Name and Data (for custom data) fields to HostFunctionCallContext #184

Closed

codefromthecrypt reviewed Feb 7, 2022

View reviewed changes

wasm/store.go Outdated Show resolved Hide resolved

pkedy added 6 commits February 6, 2022 21:31

Added CallFunctionContext to pass a context.Context through to host c…

8917743

…alls Signed-off-by: Phil Kedy <phil.kedy@gmail.com>

Removing CallFunctionContext in favor of passing context in CallFunct…

cae0e50

…ion (breaking change)

CallFunction: ensure context has a non-nil value.

0831f81

In NewHostFunctionCallContext, exposing context via a Context() fun…

53a0abf

…c instead of embedding it.

Tweak

afb31e6

Add test for nil context passed to CallFunction

fb45917

Signed-off-by: Phil Kedy <phil.kedy@gmail.com>

pkedy force-pushed the call_function_context branch from e5ce7f9 to fb45917 Compare February 7, 2022 02:58

mathetake approved these changes Feb 7, 2022

View reviewed changes

Linter fixes

6de52bb

Signed-off-by: Phil Kedy <phil.kedy@gmail.com>

codefromthecrypt approved these changes Feb 7, 2022

View reviewed changes

codefromthecrypt merged commit 4819d72 into tetratelabs:main Feb 7, 2022

pkedy deleted the call_function_context branch February 7, 2022 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

propagate Go context into HostFunctionCallContext #203

propagate Go context into HostFunctionCallContext #203

pkedy commented Feb 4, 2022 •

edited

Loading

codefromthecrypt left a comment

codefromthecrypt Feb 5, 2022

codefromthecrypt Feb 5, 2022

pkedy Feb 6, 2022

codefromthecrypt Feb 6, 2022

codefromthecrypt commented Feb 5, 2022

mathetake commented Feb 5, 2022

pkedy commented Feb 6, 2022

codefromthecrypt left a comment

codefromthecrypt Feb 6, 2022

pkedy Feb 7, 2022

codefromthecrypt Feb 7, 2022

codefromthecrypt Feb 6, 2022

mathetake Feb 7, 2022

mathetake Feb 7, 2022

pkedy Feb 7, 2022

mathetake Feb 7, 2022

mathetake Feb 7, 2022

pkedy Feb 7, 2022

pkedy Feb 7, 2022

codefromthecrypt Feb 7, 2022

codefromthecrypt Feb 7, 2022

codefromthecrypt commented Feb 7, 2022

mathetake left a comment

codefromthecrypt left a comment

pkedy commented Feb 7, 2022

codefromthecrypt commented Feb 21, 2022

		@@ -314,6 +315,10 @@ func (s Store) Instantiate(module Module, name string) error {
		}

		func (s *Store) CallFunction(moduleName, funcName string, params ...uint64) (results []uint64, resultTypes []ValueType, err error) {

propagate Go context into HostFunctionCallContext #203

propagate Go context into HostFunctionCallContext #203

Conversation

pkedy commented Feb 4, 2022 • edited Loading

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Feb 5, 2022

mathetake commented Feb 5, 2022

pkedy commented Feb 6, 2022

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Feb 7, 2022

mathetake left a comment

Choose a reason for hiding this comment

codefromthecrypt left a comment

Choose a reason for hiding this comment

pkedy commented Feb 7, 2022

codefromthecrypt commented Feb 21, 2022

pkedy commented Feb 4, 2022 •

edited

Loading