core: add multiple engine support #332
Comments
|
Note that when we have support for this, we should consolidate some of the iOS integration tests into one file/case to exercise this functionality: #1298 |
|
Moving context from #2003: Per @carloseltuerto:
|
|
Per @alyssawilk's comment:
|
|
This PR #2085 pulls in all of @alyssawilk's upstream changes. |
alyssawilk
added a commit
to envoyproxy/envoy
that referenced
this issue
May 16, 2022
…21277) Risk Level: low Testing: turned off assertions in multi-envoy test Docs Changes: n/a Release Notes: n/a part of envoyproxy/envoy-mobile#332 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
ravenblackx
pushed a commit
to ravenblackx/envoy
that referenced
this issue
Jun 8, 2022
…nvoyproxy#21277) Risk Level: low Testing: turned off assertions in multi-envoy test Docs Changes: n/a Release Notes: n/a part of envoyproxy/envoy-mobile#332 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
jpsim
added a commit
that referenced
this issue
Jul 6, 2022
Until now, `EngineHandle` has always had a single value of `1`. In other words, the Envoy engine has always been a singleton with no way to run multiple independent engines. In order to support multiple concurrent engines (#332), we first need to make `EngineHandle::initEngine` create a new engine and rather than have functions that use the engine do a singleton lookup, we can use the `envoy_engine_t` as an opaque handle type to get back the `Envoy::Engine` instance. Several places were using a hardcoded value of `1`, so we need to plumb through the engine handles. With this change although you can now create multiple engine instances, it is not safe to do so because there is still some static state that will need to be untangled. Also note that like the singleton we have had until now, new engine instances that are created will leak, as there is no way to fully release the engine through the public API. This is intentional in order to reduce the risk associated with this change. Mike and I made these changes while pairing. The bulk of the ideas here are really from him. Risk Level: High. Testing: Existing tests & Lyft apps validation. Docs Changes: None. Release Notes: Added. Co-authored-by: Mike Schore <mike.schore@gmail.com> Co-authored-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>
colibie
added a commit
to colibie/envoy-mobile
that referenced
this issue
Oct 25, 2022
This was due to running two engines simultaneously which not yet fully support(Issue envoyproxy#332) Signed-off-by: Chidera Olibie <colibie@google.com>
oschaaf
pushed a commit
to maistra/envoy
that referenced
this issue
Oct 26, 2022
…(#21277) Risk Level: low Testing: turned off assertions in multi-envoy test Docs Changes: n/a Release Notes: n/a part of envoyproxy/envoy-mobile#332 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
jpsim
added a commit
to envoyproxy/envoy
that referenced
this issue
Nov 29, 2022
Until now, `EngineHandle` has always had a single value of `1`. In other words, the Envoy engine has always been a singleton with no way to run multiple independent engines. In order to support multiple concurrent engines (envoyproxy/envoy-mobile#332), we first need to make `EngineHandle::initEngine` create a new engine and rather than have functions that use the engine do a singleton lookup, we can use the `envoy_engine_t` as an opaque handle type to get back the `Envoy::Engine` instance. Several places were using a hardcoded value of `1`, so we need to plumb through the engine handles. With this change although you can now create multiple engine instances, it is not safe to do so because there is still some static state that will need to be untangled. Also note that like the singleton we have had until now, new engine instances that are created will leak, as there is no way to fully release the engine through the public API. This is intentional in order to reduce the risk associated with this change. Mike and I made these changes while pairing. The bulk of the ideas here are really from him. Risk Level: High. Testing: Existing tests & Lyft apps validation. Docs Changes: None. Release Notes: Added. Co-authored-by: Mike Schore <mike.schore@gmail.com> Co-authored-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The skeleton for this is in place, but it needs to be implemented.
Most of the library has been designed with the assumption that the
EnvoyEnginewill be instantiated (potentially multiple times) within the process. However, theEngineitself currently is held in a static weak pointer. We have a method to return an engine identifier, that currently is stubbed to simply return1.In an earlier change, I made this type pointer width, and my plan for when we fully switch over will be to actually store the memory location of the
Engineas the identifier. (This id is opaque and never actually exposed above the bridge layer.)Because of this, we won't actually need a collection to hold
Engineinstances. Instead we can create anEnginewithnew, and store it until the platform engine wrapper is deallocated. At this point, we candrainand then ultimatelydeletetheEngineinstance.With #923 resolving the last major category of shutdown crashes, the door should be open to make this change. There are a handful of locations in code where we intentionally currently leak memory as a shortcut since the
Engineitself has been static anyways. These are commented, and I'll subsequently update those comments with a reference to this issue so we can be sure to clean them up.The text was updated successfully, but these errors were encountered: