core: separate internal dispatch from http path

[goaway]

Description: Previously the Http::Dispatcher served the dual purpose of dispatching incoming library calls onto Envoy's main thread event loop and also functioning as the HTTP client. This conflation led to a bunch of incidental complexity, including that the Http::Dispatcher needed to exist as soon as the Engine did, but before Envoy was actually up and running and could provide the interfaces the Http::Dispatcher would need to call to actually accomplish anything.

This change splits the logic into an Http::Client that is largely single-threaded code and contains no synchronization/locking and an Event::ProvisionalDispatcher that wraps Envoy's internal Event::Dispatcher but can persist beyond (before and after) the underlying dispatcher's lifecycle.

The change simplifies the dispatch and threading model of incoming calls, opens the door to resolving some types of races during Engine shutdown, and supports non-singleton integrations.

Risk Level: High
Testing: Local, Unit, Integration, E2E, CI

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

[junr03]

Thanks for these changes I think this: a) separates concerns more cleanly, b) allocating the engine on the heap and not stopping the event loop/destructing will obviously solve the pesky shutdown crashes that remain.

First pass of comments. I know you have more commits coming, so some of this might have changed already.

Also can you refresh me on the thread safety/locking conversation we had a couple weeks ago and the decisions we came to? I can't quite remember, but to me it looks like all the locking has been preserved here? Sorry I am a little foggy, the last two weeks where a whirlwind as you know 😅

[goaway]

@junr03 please take a look and comment on approach (still needs fixed tests and cleanup).

[junr03]

Awesome clean up.

re: approach -- it matches the synchronization model we had before just more cleanly thanks to logic separation in two (more) appropriate classes.

Per my previous comment, I thought we had discussed potentially going lock-free here... but I cannot remember the solution we arrived to. Not necessarily saying we should go there in this PR. I would just like it documented in an issue. In fact, it is probably best to land this, and get it out as it doesn't change the model and has less change for unforeseen edge cases, specially since tests should largely be the same as before just rejiggered for the class separation.

[junr03]

Just a few comments.

I think a format run is missing like you said.

[junr03]

some comments. I can't quite see from reviewing what is causing the ASAN and TSAN failures. I will take a look at the CI failures in more detail.

[junr03]

Alright, a couple clean up things. Then lets merge!