proxy-wasm-rust-sdk icon indicating copy to clipboard operation
proxy-wasm-rust-sdk copied to clipboard

Panic on `duplicate token_id` when using multiple root contexts

Open krdln opened this issue 3 years ago • 6 comments

What I've done:

  • instantiated multiple filters from the same wasm module
    • each running on different listener
    • sharing the same vm (I didn't override vm_id)
  • the filters had different root_id (thus different RootContexts)
  • all filters were creating grpc callouts

What happened:

  • panic on this line https://github.com/proxy-wasm/proxy-wasm-rust-sdk/blob/abd0f5437212e5fd3dd6a70eac3959934278e643/src/dispatcher.rs#L124

Suspected root cause: Filters have separate RootContexts and thus Envoy has two separate Context objects for them. Each Context has it's own grpc token counter, but proxy-wasm has single single map for tokens and the context id is ignored in the callback: https://github.com/proxy-wasm/proxy-wasm-rust-sdk/blob/abd0f5437212e5fd3dd6a70eac3959934278e643/src/dispatcher.rs#L691-L694 This results in token ids conflicts.

I don't have a repro, as the issue was discovered in a private codebase and the the panic was occuring randomly, sorry.

krdln avatar Aug 10 '21 10:08 krdln

  • instantiated multiple filters from the same wasm module
    • each running on different listener
    • sharing the same vm (I didn't override vm_id)

I think that's the culprit - the change to instantiate multiple versions of the same plugin is relatively recent, and the original code didn't account for that.

  • the filters had different root_id (thus different RootContexts)

Rust SDK doesn't support multiple root_ids. What's your use case? I'm asking, because support for it might be going away in the future.

Suspected root cause: Filters have separate RootContexts and thus Envoy has two separate Context objects for them. Each Context has it's own grpc token counter, but proxy-wasm has single single map for tokens and the context id is ignored in the callback: https://github.com/proxy-wasm/proxy-wasm-rust-sdk/blob/abd0f5437212e5fd3dd6a70eac3959934278e643/src/dispatcher.rs#L691-L694

This results in token ids conflicts. I don't have a repro, as the issue was discovered in a private codebase and the the panic was occuring randomly, sorry.

Thanks for investigating! That is indeed the issue, and those tokens should be unique per WasmVM: https://github.com/proxy-wasm/proxy-wasm-cpp-host/issues/185

PiotrSikora avatar Aug 10 '21 10:08 PiotrSikora

Thanks for quick response!

  • the filters had different root_id (thus different RootContexts)

Rust SDK doesn't support multiple root_ids.

TBH, I wasn't aware of that. I just needed to instantiate the same filter on different listeners, and I've set root_id because I felt it made sense :upside_down_face:

What's your use case? I'm asking, because support for it might be going away in the future.

I don't strictly need it, although without separate root contexts one needs to ask for get_property(["plugin_name"]) to differentiate between filters. With separate root contexts it was slightly easier, as different filters had different parent contexts. But that's minor, I guess.

krdln avatar Aug 10 '21 10:08 krdln

TBH, I wasn't aware of that. I just needed to instantiate the same filter on different listeners, and I've set root_id because I felt it made sense 🙃

I don't strictly need it, although without separate root contexts one needs to ask for get_property(["plugin_name"]) to differentiate between filters. With separate root contexts it was slightly easier, as different filters had different parent contexts. But that's minor, I guess.

Could you elaborate on why do you need to differentiate them?

Also, since it doesn't work, you probably didn't need it after all.

PiotrSikora avatar Aug 12 '21 03:08 PiotrSikora

Could you elaborate on why do you need to differentiate them?

The two instances of the filter are running on two separate listeners. I'm making some GRPC calls to external service, which needs to know which instance of the filter is making a call (to know on which listener the HTTP stream is happening). I mean, that's still doable, as I'm able to set different plugin names for both plugins (or in the other extreme – assign them to separate VMs).

krdln avatar Aug 12 '21 09:08 krdln

@krdln I believe the original issue should be fixed via https://github.com/proxy-wasm/proxy-wasm-cpp-host/pull/186 and https://github.com/envoyproxy/envoy/pull/17692. If you could upgrade and verify that it doesn't happen anymore, that would be great. Thanks!

PiotrSikora avatar Aug 19 '21 19:08 PiotrSikora

@PiotrSikora Yup, testing with envoy master makes this issue disappear :+1:

krdln avatar Sep 16 '21 12:09 krdln