So I do think this will be racy. The raw pointer is set and read from different threads, so there's at least a window where threads calling flush_stats may not see the server even though it's been set. The only time I think it would crash though is potentially on teardown. However, we've spent some time chasing down shutdown crashes, so I don't know if we really want to add to that risk.

I would suggest leveraging the http_dispatcher and its mutex for accessing Envoy's event_dispatcher instead for now, since I think this is a risky pattern to promulgate without further protections. Additionally, I think we should limit the surface area of cross-thread interactions just to make it easier to reason about logic and improve safety.

Long-term I'd like to split the business logic of encoding/decoding HTTP requests and handling dispatch into two separate entities, instead of having a single HTTPDispatcher.

I agree with the long term vision of splitting concerns in the http::dispatcher.

And with the fact that we might not get a flush due to visibility ordering of the server pointer (although, not sure if we should be concerned about a stronger guarantee there?)

But I don't think I agree with tacking more on to the http dispatcher. We still need a reference to the Server::Instance somewhere because it is through that interface that we flush stats. I am not sure it makes sense to make the http::dispatcher do that. Because at that point we are really making the http::dispatcher be a kitchen sync.

I wholeheartedly agree with Additionally, I think we should limit the surface area of cross-thread interactions just to make it easier to reason about logic and improve safety. And I would like to think how we can do that but without just accumulating everything in the same class.

I get the kitchen sink argument and I covered a little bit more in a slack thread, but I think that routing things through the dispatcher right now will actually make it easier to port things over to a dedicated cross-thread dispatch mechanism later, since everything that needs to be ported in the same place.

I think my main two concerns remain:

1. I think there are probably circumstances this can crash.
2. I think there's risk in needing to track down unguarded multi-threaded access later when we do clean things up.

With respect to 2), right now every multi-threaded access does occur right now through a safe mechanism. Exceptional cases like preferred_network are atomic, and primary cases use the core mutexes inside the Dispatcher. This will be the first case of unguarded access to a raw pointer, which I think is something to be avoided in and of itself.

I'm a bit less worried about (temporary) cruft in the dispatcher, because everything we can clean up once dispatch is split out will be located in one place.

Okay, I think the risks both perspectives cite are probably not that big. I still think there's a chance we could crash due to unsafe access here, but hey, also we might not. Since you're not here to discuss in person @junr03 you get to win this one by default. ;)

I'm going to file an issue about splitting out a generalized interface for cross-thread access, and this server_ ref can be flagged with that issue.

#720

This was a good discussion to have 😃 I went ahead and linked the issue inline in the code so we can update after

Sorry I disappeared, it was late here!

re: 1 and 2 above.

1. I don't think there is a risk (famous last words) because of the a) weak_ptr semantics that you introduced @goaway, and b) the engine holding ownership of main_common, which in turn holds ownership of the server object. This means that as long as we are able to lock the engine, we will have a valid server pointer. Right?

2. Yep, I think we have probably reached a point with out accesses that we need to straighten things out in a more digestible way. Thanks for filing http and threading: split Http::Dispatcher monolith into parts #720, looking forward to chatting about that when I am back.

Thanks for linking the issue inline @rebello95