Add explicit context_id parameter to all hostcalls.#92
Add explicit context_id parameter to all hostcalls.#92PiotrSikora wants to merge 3 commits intoproxy-wasm:mainfrom
context_id parameter to all hostcalls.#92Conversation
While there, add and consistently use `UNKNOWN_RESOURCE_ID` error for when resources are accessed using an unknown ID. Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
| * returns: | ||
| - `i32 (`[`proxy_status_t`]`) status` | ||
|
|
||
| > **Warning** | ||
| > This function has been deprecated in favor of explicit context IDs. | ||
|
|
||
| Changes the effective context to `context_id`. | ||
|
|
||
| This can be used to change active context, e.g. during |
There was a problem hiding this comment.
Perhaps it should be completely removed?
There was a problem hiding this comment.
It == proxy_set_effective_context, since we're explicitly passing the context IDs.
Thoughts?
There was a problem hiding this comment.
I see no reason to keep this method around.
It'll still be exported by cpp-host for plugins using older versions of the ABI, it just won't be used by the new ABI version.
|
|
||
| | Identifier | Name | Values | Deprecated | | ||
| |:----------:|:--------------------------------|:---------------------------------------------------------------------------------------------------|:----------:| | ||
| | 0x1001 | `HOST_TRACKS_ACTIVE_CONTEXT_ID` | When `1`, `context_id` can be set to `0`, which then indicates host-tracked active context ID. | Yes | |
There was a problem hiding this comment.
I originally envisioned this as a helpful step to ease the migration (i.e. SDKs could simply pass 0 as context_id and rely on hosts doing the tracking), but I'm not sure if that's worth it?
There was a problem hiding this comment.
Would the alternative be for SDKs to track and pass explicit context IDs from the get go, as part of initial support for the new ABI version? Unless you think there are tricky cases where it would be hard for an SDK to track, ISTM it's not too much work for SDKs to do as part of the switchover.
There was a problem hiding this comment.
Yes, that's the alternative.
I think I was originally worried about plugins directly calling hostcalls, and wanted to avoid adding extra code paths to each hostcall to retrieve SDK-tracked context ID, and instead rely on the existing host-side tracking... But I don't have strong opinion here, and perhaps we don't care about plugins that call the hostcalls directly, so we can drop this.
There was a problem hiding this comment.
IMO plugins that break out of the SDK are on their own to get the context ID from the SDK (though we should provide getters for the context id in each context).
|
|
||
| | Identifier | Name | Values | Deprecated | | ||
| |:----------:|:--------------------------------|:---------------------------------------------------------------------------------------------------|:----------:| | ||
| | 0x1001 | `HOST_TRACKS_ACTIVE_CONTEXT_ID` | When `1`, `context_id` can be set to `0`, which then indicates host-tracked active context ID. | Yes | |
There was a problem hiding this comment.
Would the alternative be for SDKs to track and pass explicit context IDs from the get go, as part of initial support for the new ABI version? Unless you think there are tricky cases where it would be hard for an SDK to track, ISTM it's not too much work for SDKs to do as part of the switchover.
|
|
||
| * params: | ||
| - none | ||
| - `i32 (uint32_t) context_id` |
There was a problem hiding this comment.
In hostcalls like this one, which list their param as context_id rather than stream_context_id or plugin_context_id, is it correct to interpret that as meaning that either type of context ID can be passed?
For this hostcall in particular, would passing plugin_context_id to proxy_done be a way of signalling shutdown of the plugin as a whole?
There was a problem hiding this comment.
Sorry, I think I was just being lazy here. proxy_done can only be called with plugin_context_id to signal shutdown of the plugin, that's the only function of this hostcall. It's not supported with stream_context_id.
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
| ## Timers | ||
|
|
||
| > **Note** | ||
| > The default timer can be accessed using `timer_id=0`. |
There was a problem hiding this comment.
This and the default shared KV store (kvstore_id=0) and queue (queue_id=0) are added to ease migration from single instance model in the current ABI, and to simplify plugins that don't require multiple instances, but perhaps we don't need those default instances?
leonm1
left a comment
There was a problem hiding this comment.
My understanding now is that the wasm code itself tells the host which context to use via the context_id parameter. Previously, it was implicit based on the last wasm callback the host invoked.
The context id is given to the wasm code during proxy_on_context_create, then is it the responsibility of the SDK to keep track of it, or is it on plugin authors? It would be helpful guidance to explain this in the description. Can you expand the PR description a bit to express what is changing with this spec change?
| * returns: | ||
| - `i32 (`[`proxy_status_t`]`) status` | ||
|
|
||
| > **Warning** | ||
| > This function has been deprecated in favor of explicit context IDs. | ||
|
|
||
| Changes the effective context to `context_id`. | ||
|
|
||
| This can be used to change active context, e.g. during |
There was a problem hiding this comment.
I see no reason to keep this method around.
It'll still be exported by cpp-host for plugins using older versions of the ABI, it just won't be used by the new ABI version.
|
|
||
| | Identifier | Name | Values | Deprecated | | ||
| |:----------:|:--------------------------------|:---------------------------------------------------------------------------------------------------|:----------:| | ||
| | 0x1001 | `HOST_TRACKS_ACTIVE_CONTEXT_ID` | When `1`, `context_id` can be set to `0`, which then indicates host-tracked active context ID. | Yes | |
There was a problem hiding this comment.
IMO plugins that break out of the SDK are on their own to get the context ID from the SDK (though we should provide getters for the context id in each context).
|
|
||
| When `parent_context_id` is `0` then a new plugin context is created, | ||
| otherwise a new per-stream context is created and `parent_context_id` | ||
| refers to the plugin context. |
There was a problem hiding this comment.
Can you confirm that context_id provided here by the host is what is to be passed as a parameter to all the hostcalls?
While there, add and consistently use
UNKNOWN_RESOURCE_IDerrorfor when resources are accessed using an unknown ID.