envoy-mobile
envoy-mobile copied to clipboard
envoyproxy
core: add multiple engine support
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 EnvoyEngine will be instantiated (potentially multiple times) within the process. However, the Engine itself currently is held in a static weak pointer. We have a method to return an engine identifier, that currently is stubbed to simply return 1.
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 Engine as 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 Engine instances. Instead we can create an Engine with new, and store it until the platform engine wrapper is deallocated. At this point, we can drain and then ultimately delete the Engine instance.
With https://github.com/lyft/envoy-mobile/pull/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 Engine itself 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.
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: https://github.com/envoyproxy/envoy-mobile/pull/1298
Moving context from #2003:
Per @carloseltuerto:
Today, due to some EnvoyProxy limitations, Envoy-Mobile can not run more than one Engine at a given moment. For the YouTube App, only one Engine is not viable - it needs at least two concurrent Engines.
TODO: upstream tests with tear down of A first, tear down of B first, different logging paths.
Per @alyssawilk's comment:
Ok @goaway @junr03 the upstream part is "done enough" insofar as if you bump main it should work (and if not, it definitely works with envoy.restart_features.no_runtime_singleton = true, as that's what is regression tested)
This PR https://github.com/envoyproxy/envoy-mobile/pull/2085 pulls in all of @alyssawilk's upstream changes.
Oh nice, this one has the details of what needs to be done. Thanks for filing it @goaway and digging it up, @junr03
