Skip to content

thread: remove ThreadFactorySingleton #6658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 3 commits into from
Apr 20, 2019
Merged

thread: remove ThreadFactorySingleton #6658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0ed5a1
Remove ThreadFactorySingleton.
AndresGuedez Apr 19, 2019
c2b31ec
Merge remote-tracking branch 'upstream/master' into thread-factory-si…
AndresGuedez Apr 19, 2019
2383bd9
Add comment clarifying dependency injection via member function.
AndresGuedez Apr 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 0 additions & 28 deletions include/envoy/thread/thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,34 +51,6 @@ class ThreadFactory {
virtual ThreadIdPtr currentThreadId() PURE;
};

/**
* A static singleton to the ThreadFactory corresponding to the build platform.
*
* The singleton must be initialized via set() early in main() with the appropriate ThreadFactory
* (see source/exe/{posix,win32}/platform_impl.h).
*
* This static singleton is an exception to Envoy's established practice for handling of singletons,
* which are typically registered with and accessed via the Envoy::Singleton::Manager. Reasons for
* the exception include drastic simplification of thread safety assertions; e.g.:
* ASSERT(ThreadFactorySingleton::get()->currentThreadId() == original_thread_id_);
*/
class ThreadFactorySingleton {
public:
/**
* Returns a reference to the platform dependent ThreadFactory.
*/
static ThreadFactory& get() { return *thread_factory_; }

/**
* Sets the singleton to the supplied thread_factory.
* @param thread_factory the ThreadFactory instance to be pointed to by this singleton.
*/
static void set(ThreadFactory* thread_factory);

private:
static ThreadFactory* thread_factory_;
};

/**
* Like the C++11 "basic lockable concept" but a pure virtual interface vs. a template, and
* with thread annotations.
Expand Down
18 changes: 0 additions & 18 deletions source/common/thread/BUILD
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions source/common/thread/thread_factory_singleton.cc
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion source/exe/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ envoy_cc_library(
"//source/common/http/http2:codec_lib",
"//source/common/common:perf_annotation_lib",
"//source/common/stats:fake_symbol_table_lib",
"//source/common/thread:thread_factory_singleton_lib",
"//source/server:hot_restart_lib",
"//source/server:hot_restart_nop_lib",
"//source/server:proto_descriptors_lib",
Expand Down
6 changes: 1 addition & 5 deletions source/exe/main_common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti
Filesystem::Instance& file_system)
: options_(options), component_factory_(component_factory), thread_factory_(thread_factory),
file_system_(file_system) {
Thread::ThreadFactorySingleton::set(&thread_factory_);
ares_library_init(ARES_LIB_INIT_ALL);
Event::Libevent::Global::initialize();
RELEASE_ASSERT(Envoy::Server::validateProtoDescriptors(), "");
Expand Down Expand Up @@ -98,10 +97,7 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti
}
}

MainCommonBase::~MainCommonBase() {
Thread::ThreadFactorySingleton::set(nullptr);
ares_library_cleanup();
}
MainCommonBase::~MainCommonBase() { ares_library_cleanup(); }

void MainCommonBase::configureComponentLogLevels() {
for (auto& component_log_level : options_.componentLogLevels()) {
Expand Down
12 changes: 11 additions & 1 deletion source/extensions/quic_listeners/quiche/platform/quic_thread_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ class QuicThreadImpl {
}

void Start() {
ASSERT(thread_factory_ != nullptr);
if (thread_ != nullptr || thread_is_set_.HasBeenNotified()) {
PANIC("QuicThread can only be started once.");
}
thread_ = Envoy::Thread::ThreadFactorySingleton::get().createThread([this]() {
thread_ = thread_factory_->createThread([this]() {
thread_is_set_.WaitForNotification();
this->Run();
});
Expand All @@ -48,6 +49,14 @@ class QuicThreadImpl {
thread_ = nullptr;
}

// Sets the thread factory to use.
// NOTE: The factory can not be passed via a constructor argument because this class is itself a
// dependency of an external library that derives from it and expects a single argument
// constructor.
void setThreadFactory(Envoy::Thread::ThreadFactory& thread_factory) {
Copy link
Contributor

@jmarantz jmarantz Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AndresGuedez AndresGuedez Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jmarantz jmarantz Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

thread_factory_ = &thread_factory;
}

protected:
virtual void Run() {
// We don't want this function to be pure virtual, because it will be called if:
Expand All @@ -61,6 +70,7 @@ class QuicThreadImpl {

private:
Envoy::Thread::ThreadPtr thread_;
Envoy::Thread::ThreadFactory* thread_factory_;
Copy link
Contributor

@wu-bin wu-bin Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor

@jmarantz jmarantz Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@AndresGuedez AndresGuedez Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@AndresGuedez AndresGuedez Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mpwarres mpwarres Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jmarantz jmarantz Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@wu-bin wu-bin Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jmarantz jmarantz Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mattklein123 mattklein123 Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

absl::Notification thread_is_set_; // Whether |thread_| is set in parent.
};

Expand Down
1 change: 0 additions & 1 deletion test/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ envoy_cc_test_library(
"//source/common/common:thread_lib",
"//source/common/event:libevent_lib",
"//source/common/http/http2:codec_lib",
"//source/common/thread:thread_factory_singleton_lib",
"//test/common/runtime:utility_lib",
"//test/mocks/access_log:access_log_mocks",
"//test/test_common:environment_lib",
Expand Down
18 changes: 0 additions & 18 deletions test/common/thread/BUILD
View file
Open in desktop

This file was deleted.

35 changes: 0 additions & 35 deletions test/common/thread/thread_factory_singleton_test.cc
View file
Open in desktop

This file was deleted.

15 changes: 1 addition & 14 deletions test/exe/main_common_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,20 +40,7 @@ class MainCommonTest : public testing::TestWithParam<Network::Address::IpVersion
TestEnvironment::PortMap(), GetParam())),
random_string_(fmt::format("{}", computeBaseId())),
argv_({"envoy-static", "--base-id", random_string_.c_str(), "-c", config_file_.c_str(),
nullptr}) {
// The test main() sets the ThreadFactorySingleton since it is required by all other tests not
// instantiating their own MainCommon.
// Reset the singleton to a nullptr to avoid triggering an assertion when MainCommonBase() calls
// set() in the tests below.
Thread::ThreadFactorySingleton::set(nullptr);
}

~MainCommonTest() override {
// This is ugly, but necessary to enable a stronger ASSERT() in ThreadFactorySingleton::set().
// The singleton needs to be reset to a non nullptr value such that when the constructor runs
// again, the ThreadFactorySingleton::set(nullptr) does not trigger the assertion.
Thread::ThreadFactorySingleton::set(&Thread::threadFactoryForTest());
}
nullptr}) {}

/**
* Computes a numeric ID to incorporate into the names of
Expand Down
3 changes: 2 additions & 1 deletion test/extensions/quic_listeners/quiche/platform/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ licenses(["notice"]) # Apache 2
load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_fuzz_test",
"envoy_cc_platform_dep",
"envoy_cc_test",
"envoy_cc_test_binary",
"envoy_cc_test_library",
Expand Down Expand Up @@ -40,7 +41,7 @@ envoy_cc_test(
"//test/test_common:utility_lib",
"@com_googlesource_quiche//:quic_platform_port_utils",
"@com_googlesource_quiche//:quic_platform_sleep",
],
] + envoy_cc_platform_dep("//source/exe:platform_impl_lib"),
)

envoy_cc_test(
Expand Down
16 changes: 11 additions & 5 deletions test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include "common/memory/stats.h"
#include "common/network/utility.h"

#include "exe/platform_impl.h"

#include "test/common/stats/stat_test_utility.h"
#include "test/extensions/transport_sockets/tls/ssl_test_utility.h"
#include "test/mocks/api/mocks.h"
Expand Down Expand Up @@ -252,10 +254,14 @@ TEST_F(QuicPlatformTest, QuicStringPiece) {
}

TEST_F(QuicPlatformTest, QuicThread) {
Envoy::PlatformImpl platform_impl;

class AdderThread : public QuicThread {
public:
AdderThread(int* value, int increment)
: QuicThread("adder_thread"), value_(value), increment_(increment) {}
AdderThread(int* value, int increment, Envoy::Thread::ThreadFactory& thread_factory)
: QuicThread("adder_thread"), value_(value), increment_(increment) {
setThreadFactory(thread_factory);
}

~AdderThread() override = default;

Expand All @@ -270,19 +276,19 @@ TEST_F(QuicPlatformTest, QuicThread) {
int value = 0;

// A QuicThread that is never started, which is ok.
{ AdderThread t0(&value, 1); }
{ AdderThread t0(&value, 1, platform_impl.threadFactory()); }
EXPECT_EQ(0, value);

// A QuicThread that is started and joined as usual.
{
AdderThread t1(&value, 1);
AdderThread t1(&value, 1, platform_impl.threadFactory());
t1.Start();
t1.Join();
}
EXPECT_EQ(1, value);

// QuicThread will panic if it's started but not joined.
EXPECT_DEATH_LOG_TO_STDERR({ AdderThread(&value, 2).Start(); },
EXPECT_DEATH_LOG_TO_STDERR({ AdderThread(&value, 2, platform_impl.threadFactory()).Start(); },
"QuicThread should be joined before destruction");
}

Expand Down
1 change: 0 additions & 1 deletion test/main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ int main(int argc, char** argv) {
// Enabled by default. Control with "bazel --define=signal_trace=disabled"
Envoy::SignalAction handle_sigs;
#endif
Envoy::Thread::ThreadFactorySingleton::set(&Envoy::Thread::threadFactoryForTest());

Envoy::TestEnvironment::setEnvVar("TEST_RUNDIR",
(Envoy::TestEnvironment::getCheckedEnvVar("TEST_SRCDIR") + "/" +
Expand Down