iOS: fix termination crash in ProvisionalDispatcher (again)#2276
Conversation
I believe #2059 helped reduce these occurrences, but we're still seeing some termination crashes where a different thread is waiting on `std::thread::join()`. So moving this to before the `main_thread_.join()` may help. Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
snowp
left a comment
There was a problem hiding this comment.
What data race are you concerned about? Seems like this is all done under a lock
I wonder if it would be helpful to set up a integration test which keeps issuing calls against the dispatcher while shutting down, then using bazels "run this test 1000 times" feature to see if we're still hitting crashes
| event_dispatcher_->exit(); | ||
| dispatcher_->terminate(); |
There was a problem hiding this comment.
Is the provisional dispatcher valid after event_dispatcher_->exit() is called? Should the order here be flipped?
There was a problem hiding this comment.
I'd actually suggest moving the exit() call inside of ProvisionalDispatcher::terminate() to make this less delicate.
There was a problem hiding this comment.
I've reverted it to what it was before. Moving the exit into terminate() caused the java tests to crash:
#
# A fatal error has been detected by the Java Runtime Environment:
#
# SIGSEGV (0xb) at pc=0x000000014156e5d9, pid=28881, tid=29959
#
# JRE version: OpenJDK Runtime Environment Zulu11.50+19-CA (11.0.12+7) (build 11.0.12+7-LTS)
# Java VM: OpenJDK 64-Bit Server VM Zulu11.50+19-CA (11.0.12+7-LTS, mixed mode, tiered, compressed oops, g1 gc, bsd-amd64)
# Problematic frame:
# C [libndk_envoy_jni.jnilib+0x1fee5d9] Envoy::Event::ProvisionalDispatcher::terminate()+0x29
If you look at the logic in Thanks @jpsim! |
goaway
left a comment
There was a problem hiding this comment.
See minor suggestion above.
…atcher-again * main: envoy: update to d88f31b (#2279) api: disallow setting 'host' header directly (#2275) android: add support for registering a platform KV store (#2134) Bump Lyft Support Rotation (#2278) tools: Enable the VSCode completion db to use bazelisk if available (#2277) Signed-off-by: JP Simard <jp@jpsim.com>
To the provisional dispatcher's terminate Signed-off-by: JP Simard <jp@jpsim.com>
This reverts commit e9d1254. Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
…rash-in-provisionaldispatcher-again * origin/main: tools: use `bazelw` bazel wrapper in `vscode_compdb.sh` (#2284) ci: add scheduled job to update Envoy weekly (#2282) docs: update python packages to work with Python 3.10 (#2286) test: adding a cancel test, cleaning up copy-paste code (#2283) Signed-off-by: JP Simard <jp@jpsim.com>
I believe #2059 helped reduce these occurrences, but we're still seeing some termination crashes where a different thread is waiting on
std::thread::join().So moving this to before the
main_thread_.join()may help.Risk Level: Moderate, there's a possibility this will introduce a data race, but even if that's the case, it'll be when the engine is being terminated.
Release Notes: Added
Co-authored-by: Mike Schore mike.schore@gmail.com
Signed-off-by: JP Simard jp@jpsim.com