feat: add GetRootContextByID call #93
Conversation
There was a problem hiding this comment.
nit: I agree this function maybe useful, but you can manage root_context_id -> context_id mapping in your code because the parent root_id is given in callbacks set by SetNewHttpContext and SetNewStreamContext. If you use your own mappings, you don't need to cast proxywasm.RootContext (returned by this function) to your specific struct.
sorry, I was mistaken. nvm
wouterposdijk
force-pushed
the
add-root-context-call
branch
from
5adf29a to
24b6fd7
Compare
proxywasm/vmstate.go
Outdated
| func GetRootContextByID(rootContextID uint32) (RootContext, error) { | ||
| rootContextState, ok := currentState.rootContexts[rootContextID] | ||
| if !ok { | ||
| return nil, types.ErrorStatusNotFound |
There was a problem hiding this comment.
types.ErrorStatusNotFound corresponds to the error status code returned by the "host" in any hostcall
- https://github.com/tetratelabs/proxy-wasm-go-sdk/blob/main/proxywasm/types/types.go#L67
- https://github.com/proxy-wasm/proxy-wasm-cpp-host/blob/master/src/context.cc#L74
so I think we can just do like:
| return nil, types.ErrorStatusNotFound | |
| return nil, fmt.Errorf("root context not found: %d", rootContextID) |
There was a problem hiding this comment.
Maybe it would be better to define a static error then:
var ErrorRootContextNotFound = errors.New("root context not found")
and then wrap it with fmt.Errorf("%w: %d", ErrorRootContextNotFound, rootContextID)? That way, the error can be identified by the caller using errors.Is.
If you agree: I'm not sure where you would want to place this static error, though; there does not seem to be a standard for it in this project yet. Do you have a preference, or should I just place it above the function?
There was a problem hiding this comment.
agree with static error! Go ahead 👍
There was a problem hiding this comment.
Alright, I've added it. I've also added a unit test, unfortunately I can't seem to get the e2e tests to run locally.
wouterposdijk
force-pushed
the
add-root-context-call
branch
from
24b6fd7 to
200cab6
Compare
wouterposdijk
force-pushed
the
add-root-context-call
branch
from
44de166 to
67e6b55
Compare
This is kind of an improvement from #93 in favor of proxy-wasm/proxy-wasm-rust-sdk#34 and proxy-wasm/proxy-wasm-rust-sdk#67 Note that this is a breaking change, though the fix is trivial. Signed-off-by: Takeshi Yoneda [email protected]
I would like to add this GetRootContextByID call, so that configuration stored in the root context can be read by the child context.