document or change the fact that response callbacks for outbound HTTP / gRPC calls are performed in the root context

[mpwarres]

Currently, if a plugin initiated an outbound gRPC or HTTP call, the response to that call is dispatched in a callback for which the effective context is the root context rather than the initiating stream context. This means that if callout response callback code attempts to perform an operation that is dependent on a stream context, such as sendLocalResponse (see envoyproxy/envoy#39797), that operation will fail.

We should either:

A. Change the default behavior of HTTP / gRPC response callbacks to set the initiating stream context as the effective context, or

B. Clearly document the current behavior, and suggest that plugins call Context::setEffectiveContext() if they want to reassert the stream context in the response callback.

(A) is a behavior change, so comes with some risk of breaking plugins that depend on the current behavior. Per Chesterton's fence, it would also be good to understand better the existing choice of default before changing it. @kyessenov suggested this might be because one of the initial uses of gRPC callouts (for telemetry export) wanted to batch data across streams. @PiotrSikora do you remember any other considerations?

[PiotrSikora]

(A) would break all existing plugins using HTTP / gRPC callouts, since they expect response on the RootContext, so that's a big no-no.

I don't recall why this is implemented the way it is, but there are valid use cases where you want callouts to outlive the lifetime of the primary connection or request (e.g. remote metrics / logging), so they have to be associated with the plugin context, and I suspect that letting the plugin authors control this instead of trying to guess the intent on the host side led to this design.

There are also plugin-level callouts (e.g. aggregated metrics / logging).

But this is something that can (should?) be handled in the SDK (see: Rust SDK), there is no need to leak this into plugin-land.

[mpwarres]

I don't recall why this is implemented the way it is, but there are valid use cases where you want callouts to outlive the lifetime of the primary connection or request (e.g. remote metrics / logging), so they have to be associated with the plugin context, and I suspect that letting the plugin authors control this instead of trying to guess the intent on the host side led to this design.

Thanks, this is useful background, consistent with what @kyessenov had mentioned in offline conversation.

But this is something that can (should?) be handled in the SDK (see: Rust SDK), there is no need to leak this into plugin-land.

I agree it can be handled entirely in the SDK. I wasn't sufficiently clear, but in (A) I was imagining that the setEffectiveContext() call would be in the C++ SDK, e.g. proxy_on_grpc_receive would become something like:

extern "C" PROXY_WASM_KEEPALIVE void proxy_on_grpc_receive(uint32_t context_id, uint32_t token,
                                                           uint32_t response_size) {
  if (Context* context = getContext(context_id); context != nullptr) {
    context->setEffectiveContext();
  }
  getRootContext(context_id)->onGrpcReceive(token, static_cast<size_t>(response_size));
}

which IIUC would be roughly equivalent to the Rust SDK behavior you linked to above.

[PiotrSikora]

Ah, OK. I completely misunderstood your initial message.

The snippet you suggested makes sense, and the only downside of doing it would be a duplicated hostcall (therefore, unnecessary context switch) if the plugin already calls setEffectiveContext (as it has to right now).