-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
engine: return new handles from EngineHandle::initEngine
#2129
Conversation
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, 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. Co-authored-by: Mike Schore <mike.schore@gmail.com> Co-authored-by: JP Simard <jp@jpsim.com> Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
The following compiles cleanly: ./bazelw build android_dist --config=android Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
To `sendDataByteArray` to avoid having to match the mangled method name to go through JNI. Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
To see what else may be failing. Signed-off-by: JP Simard <jp@jpsim.com>
Interestingly I don't see the asan leaks locally, only on CI:
|
Even though it's intentional to leak engine instances through the ObjC/Swift/Java/Kotlin interfaces. Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Disabling the two failing tests. Signed-off-by: JP Simard <jp@jpsim.com>
Ignoring failing tests for now. Signed-off-by: JP Simard <jp@jpsim.com>
This reverts commit 6e6b81b. Signed-off-by: JP Simard <jp@jpsim.com>
By running `tearDown()` between invocations. Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
This reverts commit 43ae8ca. Signed-off-by: JP Simard <jp@jpsim.com>
This reverts commit dcc64dd. Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
These are already being filtered out of our stats inclusion list, so this isn't a user-facing change, but this is needed to support the singleton removal work in #2129. Splitting this out into its own PR to reduce the scope of the singleton removal change. Signed-off-by: JP Simard <jp@jpsim.com>
* origin/main: (33 commits) iOS: fix xcframework upload in release workflow (#2366) config: hopefully fixing C++ config default for apple (#2355) Update Envoy (#2364) Bump Lyft Support Rotation (#2365) ci: pin external GitHub Action (#2363) cleanup: fix warning in JNI layer (#2361) cleanup: convert some more uses of NULL to nullptr (#2359) cleanup: consistently use nullptr in cc contexts (#2351) cleanup: remove unused function and resolve warning (#2350) iOS: add configurable gzip and brotli decompression options (#2349) iOS: stop embedding bitcode in releases (#2347) ci: update Android setup (#2354) docs: update the list of clusters (#2344) bazel: update rules_apple (#2346) iOS: add a way to disable network monitoring (#2345) api: adding brotli knobs (#2342) android: create persistent SharedPreferences-based KV store (#2319) ios: add support for registering a platform KV store (#2334) builder: making compressor configurable (#2321) iOS: add SwiftPM example (#2333) ... Signed-off-by: JP Simard <jp@jpsim.com>
These are already being filtered out of our stats inclusion list, so this isn't a user-facing change, but this is needed to support the singleton removal work in #2129. Splitting this out into its own PR to reduce the scope of the singleton removal change. Co-authored-by: alyssawilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
To support interop between the Swift/Kotlin and C++ APIs. Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
This reverts commit 546633e. Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
These are already being filtered out of our stats inclusion list, so this isn't a user-facing change, but this is needed to support the singleton removal work in #2129. Splitting this out into its own PR to reduce the scope of the singleton removal change. Co-authored-by: alyssawilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com> Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
* origin/main: Update Envoy to e792008a66f (#2391) swift: fix docstrings and docstring linter (#2389) ci: change submodule_update PR branch name to use a timestamp strategy (#2387) Update Envoy (#2385) bazel: bump rules_apple and rules_swift to latest releases (#2384) Bump Lyft Support Rotation (#2386) Update Envoy to 665b4d5 (#2373) config: fixing apple dns v2 (#2378) Make CI jobs depend on Envoy build CI job (#2381) release notes: update for #2379 (#2382) android: add experimental option to force all connections to use IPv6 (#2379) Bump Envoy sha to f49d4f2 (#2380) Signed-off-by: JP Simard <jp@jpsim.com>
* origin/main: Update Envoy (#2403) connectivity_manager: rename/refactor from Network::Configurator (#2401) test: fixing up build targets to use the extension registry (#2397) test: Adds an integration test for SDS (#2395) iOS: add `forceIPv6(...)` builder option (#2396) api: make iOS Headers and HeadersBuilder case-insensitive (#2383) Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
This reverts commit 31a27fe. Signed-off-by: JP Simard <jp@jpsim.com>
Should we update the description of the PR? It contains some WIP notes. |
Let's mention #332 in the description of the PR too. |
@Augustyniak I've updated the PR description to make more sense as a commit message when merging this. |
@@ -15,6 +15,9 @@ Breaking changes: | |||
- iOS: enable usage of ``NWPathMonitor`` by default (:issue:`#2329 <2329>`) | |||
- iOS: replace ``enableNetworkPathMonitor`` with a new ``setNetworkMonitoringMode`` API to allow disabling monitoring (:issue:`#2345 <2345>`) | |||
- iOS: release artifacts no longer embed bitcode | |||
- api: engines are no longer a singleton, you may need to update your code to only create engines once and hold on to them. | |||
You also cannot assume that an `envoy_engine_t` value of `1` will return the default engine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Not sure this strictly needs to be in release notes, since technically this is not a public type/interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, although it's possible to have relied on the fact that this was a predictable value from external consumers, as brittle as that may have been.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @jpsim!
These are already being filtered out of our stats inclusion list, so this isn't a user-facing change, but this is needed to support the singleton removal work in envoyproxy/envoy-mobile#2129. Splitting this out into its own PR to reduce the scope of the singleton removal change. Co-authored-by: alyssawilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>
Until now,
EngineHandle
has always had a single value of1
. 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 theenvoy_engine_t
as an opaque handle type to get back theEnvoy::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.
Description:
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