envoyproxy / envoyproxy/envoy-mobile

core: add multiple engine support

Open
#332 5 comments 0 reactions 2 assignees Claimed by @jpsim View on GitHub
core cronet cronvoy-beta no stalebot
Dominant language
Java
Stars
566
Forks
85
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.