core: envoy engine by goaway · Pull Request #498 · envoyproxy/envoy-mobile

goaway · 2019-10-11T16:18:11Z

Description: create a class to manage Envoy lifecycle and allow for graceful teardown. Teardown is now graceful for both iOS and Android. Envoy's codebase expects to have shutdown happen from the main thread. However, in mobile the thread the engine runs in is not the main thread of the process, and thus shutdown/destructors were not getting run from the thread the engine was started on. This PR makes sure that the engine is destructed/shutdown from the thread that it ran in.
Risk Level: high. This PR changes state management for the engine, and initializes it on a std::thread instead of a platform thread. iOS thread management is afaict handled gracefully. On Android the native thread has to be attached and detached from the JVM. This PR attaches the native thread, but the work to detach is a bit involved so will come in a subsequent PR.
Testing: local device testing.

Fixes #492

Co-authored-by: Jose Nino jnino@lyft.com
Signed-off-by: Mike Schore mike.schore@gmail.com

Signed-off-by: Mike Schore <mike.schore@gmail.com>

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2019-10-16T16:34:49Z

library/common/jni_interface.cc

+    // Note: the only thread that should need to be attached is Envoy's engine std::thread.
+    // TODO: harden this piece of code to make sure that we are only needing to attach Envoy
+    // engine's std::thread, and that we detach it successfully.
+    static_jvm->AttachCurrentThread(&env, NULL);


I am going to add thread detachment in a subsequent PR, as it requires introducing engine level callbacks. I don't want to make this PR any bigger.

Signed-off-by: Jose Nino <jnino@lyft.com>

buildbreaker · 2019-10-16T16:51:40Z

library/common/engine.cc

+      event_dispatcher_ = &main_common_->server()->dispatcher();
+      cv_.notifyOne();
+    } catch (const Envoy::NoServingException& e) {
+      return ENVOY_SUCCESS;


What makes NoServingException a successful run?

library/common/main_interface.cc

junr03 · 2019-10-16T17:00:06Z

@mattklein123 I am assigning you in case you can take a look, given mike (initially) and I (later) were the ones that wrote the code.

goaway · 2019-10-16T17:29:38Z

@junr03 just posting to say I think the updates you've made look good. 👍

rebello95 · 2019-10-16T20:45:57Z

Can we mention that this resolves #492 in the PR description and close out that issue when this merges? 😃

junr03 · 2019-10-16T21:38:23Z

Can we mention that this resolves #492 in the PR description and close out that issue when this merges? 😃

I wanted to to that in #507 as that completes the work for the crash 100%. It also completes work for #445. @rebello95 Does that sound good?

rebello95 · 2019-10-16T21:39:15Z

Sure, SGTM

Signed-off-by: Jose Nino <jnino@lyft.com>

goaway · 2019-10-16T22:54:22Z

Hm. I do think I'd suggest that this PR more directly addresses #492, and is probably the most relevant if we're ever looking for that commit.

junr03 · 2019-10-16T23:35:01Z

Sure, I can add it here.

mattklein123

Nice, LGTM at a high level.

mattklein123 · 2019-10-16T23:55:24Z

library/common/jni_interface.cc

+    // Note: the only thread that should need to be attached is Envoy's engine std::thread.
+    // TODO: harden this piece of code to make sure that we are only needing to attach Envoy
+    // engine's std::thread, and that we detach it successfully.
+    static_jvm->AttachCurrentThread(&env, NULL);


nit: s/NULL/nullptr

will do in subsequent PR to avoid re-approval and re-running CI

https://github.com/lyft/envoy-mobile/pull/507/files#diff-87fca06766521056096d5a45653f1aebR135

Description: this is a follow up to #498. This PR introduces `envoy_engine_callbacks`. They are similar in nature to envoy_http_callbacks. The difference being that they are not exposed all the way to consumer level in the library as it is not needed right now. However, one can see how by adding a type erased context pointer, and following the platform patterns for http callbacks we could thread this all the way up if need be. The immediate need for these callbacks is to detach the engine's native thread from the JVM on Android. Risk Level: med -- adds complexity to engine management. Testing: local testing on devices (Lyft and example app on iOS and Android). In conjunction with #498 this PR Fixes #492 #445 Signed-off-by: Jose Nino <jnino@lyft.com>

Description: #498 did not fully solve #492. The reset cleanly destructed all objects. However, because destruction was posted in to the event dispatcher, the event dispatcher was left with bad accesses. This PR fixes the issue by issuing shutdown on the dispatcher, and only destructing once the event loop has exited and control has returned to the Engine's run function. Risk Level: med - fixing crash on shutdown Testing: local Fixes #492 Signed-off-by: Jose Nino <jnino@lyft.com>

Recent changes (namely #498) made it unnecessary for the platform-level clients to start Envoy on newly spawned threads. In fact, these changes actually introduced potential races where consumers of the platform clients could end up calling `send` on engines that hadn't finished starting on these threads. This PR removes the custom threading logic in Swift in favor of simply using the new threading being done within the core. Another PR will make the same change on Android. Signed-off-by: Michael Rebello <me@michaelrebello.com>

Recent changes (namely #498) made it unnecessary for the platform-level clients to start Envoy on newly spawned threads. In fact, these changes actually introduced potential races where consumers of the platform clients could end up calling `send` on engines that hadn't finished starting on these threads. This PR removes the custom threading logic in Swift in favor of simply using the new threading being done within the core. Another PR will make the same change on Android: #531. Signed-off-by: Michael Rebello <me@michaelrebello.com>

Description: "Recent changes (namely #498) made it unnecessary for the platform-level clients to start Envoy on newly spawned threads. In fact, these changes actually introduced potential races where consumers of the platform clients could end up calling send on engines that hadn't finished starting on these threads." This PR removes the custom threading logic in Kotlin in favor of simply using the new threading being done within the core. Parallel to #530. Risk Level: med - reduce threading complexity Testing: local app. Signed-off-by: Jose Nino <jnino@lyft.com>

Recent changes (namely envoyproxy/envoy-mobile#498) made it unnecessary for the platform-level clients to start Envoy on newly spawned threads. In fact, these changes actually introduced potential races where consumers of the platform clients could end up calling `send` on engines that hadn't finished starting on these threads. This PR removes the custom threading logic in Swift in favor of simply using the new threading being done within the core. Another PR will make the same change on Android: envoyproxy/envoy-mobile#531. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

goaway changed the title ~~wip~~ core: envoy engine Oct 11, 2019

goaway force-pushed the ms/engine branch 6 times, most recently from dbee0ef to 9e77198 Compare October 11, 2019 21:23

goaway added 2 commits October 11, 2019 14:38

compiles

f3c8612

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fixes

952f2bc

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway force-pushed the ms/engine branch from 5ccb8bf to 952f2bc Compare October 11, 2019 21:39

goaway and others added 4 commits October 11, 2019 14:47

format

51eaa99

Signed-off-by: Mike Schore <mike.schore@gmail.com>

make envoy_argv non-const

dab5e45

Signed-off-by: Mike Schore <mike.schore@gmail.com>

comment out call_once

372c4a1

Signed-off-by: Mike Schore <mike.schore@gmail.com>

clean exit

c177ab4

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 marked this pull request as ready for review October 15, 2019 20:33

Jose Nino added 2 commits October 15, 2019 17:13

attach thread android

9a1130a

Signed-off-by: Jose Nino <jnino@lyft.com>

signatures

7ecafeb

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 reviewed Oct 16, 2019

View reviewed changes

Jose Nino added 2 commits October 16, 2019 09:36

docstring

5750ba2

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

258efb4

Signed-off-by: Jose Nino <jnino@lyft.com>

buildbreaker reviewed Oct 16, 2019

View reviewed changes

rebello95 reviewed Oct 16, 2019

View reviewed changes

library/common/main_interface.cc Show resolved Hide resolved

junr03 assigned mattklein123 Oct 16, 2019

junr03 mentioned this pull request Oct 16, 2019

core: engine callbacks -- add on_exit #507

Merged

comments

1cf968a

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 approved these changes Oct 16, 2019

View reviewed changes

rebello95 approved these changes Oct 17, 2019

View reviewed changes

junr03 merged commit 9133ddf into master Oct 17, 2019

rebello95 mentioned this pull request Oct 18, 2019

core/ios: crash when force-closing app with Envoy #492

Closed

junr03 mentioned this pull request Oct 19, 2019

core: fix shutdown libevent crash #527

Merged

rebello95 mentioned this pull request Oct 21, 2019

ios: use internal engine thread #530

Merged

junr03 mentioned this pull request Oct 21, 2019

android: use internal engine thread #531

Merged

goaway deleted the ms/engine branch October 23, 2019 17:57

Conversation

goaway commented Oct 11, 2019 • edited by junr03 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junr03 Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

buildbreaker Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

junr03 commented Oct 16, 2019

Uh oh!

goaway commented Oct 16, 2019

Uh oh!

rebello95 commented Oct 16, 2019

Uh oh!

junr03 commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebello95 commented Oct 16, 2019

Uh oh!

goaway commented Oct 16, 2019

Uh oh!

junr03 commented Oct 16, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

junr03 Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

junr03 Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

goaway commented Oct 11, 2019 •

edited by junr03

Loading

junr03 commented Oct 16, 2019 •

edited

Loading