thread: remove ThreadFactorySingleton

[AndresGuedez]

It is no longer needed since Api::Api is plumbed ubiquitously throughout Envoy's core.

The only user of the factory, QuicThreadImpl, has been modified to take the Envoy::Thread::ThreadFactory via QuicThreadImpl::setThreadFactory().

Signed-off-by: Andres Guedez aguedez@google.com

Risk Level: Low
Testing: All tests passing
Docs Changes: N/A
Release Notes: N/A
Fixes #5734

[AndresGuedez]

@jmarantz PTAL

[AndresGuedez]

@wu-bin / @mpwarres please review the change to QuicThreadImpl.

[jmarantz]

Just curious if it's possible to require this as a ctor arg rather than having a ctor.

[AndresGuedez]

Unfortunately no, because QuicThreadImpl is a dependency of an external dependency that is subclassing and calling the single arg constructor.

[jmarantz]

cool; might want to mention that in a comment here in case there's confusion about this pattern, but it's up to you.

[wu-bin]

Can we just initialize thread_factory_ to Envoy::PlatformImpl::threadFactory()?

To provide some context: On all QUICHE platforms(Google internal, Chromium, Envoy), the QUICHE core code needs to be able to instantiate a QuicThread using QuicThread("thread_name"), it can't use the Envoy-specific setThreadFactory().

[jmarantz]

Imo if Quic needs singletons we should isolate them to the smallest subsystem possible

If there were a Quic context that was used to instantiate threads then we could inject the thread factory into that.

[AndresGuedez]

Envoy::PlatformImpl::threadFactory() is not static.

Thanks for the context. This is an interesting use case in that Envoy is the dependency and the caller (QUICHE) would need to have Envoy specific code to plumb through the ThreadFactory to each thread instance. This would add complexity that can be avoided if we just keep the static ThreadFactorySingleton in Envoy.

@jmarantz / @mattklein123 I may be biased, but this seems like a reasonable justification for keeping the static singleton. WDYT?

[AndresGuedez]

If there were a Quic context that was used to instantiate threads then we could inject the thread factory into that.

Yeah, this seems reasonable to me as well. It's a matter of which/how many requirements Envoy will impose when used as a dependency. I agree this is a cleaner approach but requires work on QUICHE.

[mpwarres]

A similar issue to this came up in #6058, here's the discussion thread. Perhaps the Envoy QUICHE platform impl should provide a static Init() function that Envoy could pass an Api::Api into at startup (or in this case, in test fixtures). This would at least confine the singleton to the QUICHE platform impl.

[jmarantz]

Thanks for the context -- I think I saw that PR breeze by. The comments hinted that there might be a follow-up; is that pending?

My perspective is that I'd rather have the system limit as much as possible its dependence on singletons. Having a subsystem dependency that does not follow that pattern is OK..you just need the pattern suggested by @mpwarres -- call an init() function at appropriate points, e.g. some kind of global context constructor like MainCommonBase in source/exe/main_common.cc. Ideally there's also a matching terminate() function to call from its destructor.

The ThreadFactory singleton that this PR is removing is currently set/cleared there, as well as the global state needed for ares_library() and some other things. I've used this pattern a ton in the past as well, while keeping the explicit state model for the core of the system, with its benefits of easy injection, explicit dependence and initialization order, and its costs of plumbing what's needed through the constructor chains.

It would be nicer IMO if Quic would have a constructor to maintain its entire state and plumb in its platform dependencies, it is not essential and we can hook into MainCommon to initialize/terminate quic.

[wu-bin]

Both QuicThread and QuicTestOutput(added in #6058) are test-only code. They should only be linked into QUIC tests, but not non-test binaries. So, while I agree the init/terminate idea is nice, I'd like to avoid using it to initialize a thread factory, which will not be used by QUIC.

So far, in non-test QUIC core code, I have not seen a need to initialize any non-trivial globals. We probably need some in the upcoming QUIC-Envoy integration code, I'm not sure.

[jmarantz]

Thanks Bin -- that's better. If the singletons are only required in test, we have set up a hack to make that easier in test/test_common/global.h -- enabling management of test-scoped singletons.

Sounds like that's not needed for this PR though, but might be needed in the Quic tests in the future.

[mattklein123]

Big +1 from me on having there be an init style setup of the QUICHE code so that we can pass in Api and anything else we need.

[mattklein123]

Awesome