Skip to content

quiche:remove setThreadFactory() from QuicThreadImpl #6700

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 10 commits into from
Apr 26, 2019
Merged

quiche:remove setThreadFactory() from QuicThreadImpl #6700

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5783a64
remove setThreadFactory() from QuicThreadImpl
danzh1989 Apr 24, 2019
27b172c
add quic_thread_impl.cc
danzh1989 Apr 24, 2019
b2ee1d6
move thread_impl to test
danzh1989 Apr 24, 2019
4394e37
remove thread_impl.cc
danzh1989 Apr 24, 2019
1363a67
remove envoy_cc_platform_dep
danzh1989 Apr 24, 2019
7e883e3
address comment
danzh1989 Apr 25, 2019
c75da7e
Merge branch 'master' into quicthread
danzh1989 Apr 25, 2019
7ad9df4
format
danzh1989 Apr 25, 2019
acb884d
build file format
danzh1989 Apr 25, 2019
e4ae738
use reference
danzh1989 Apr 26, 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
4 changes: 3 additions & 1 deletion source/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_library",
"envoy_cc_platform_dep",
"envoy_cc_test_library",
"envoy_package",
"envoy_select_quiche",
Expand Down Expand Up @@ -79,6 +80,7 @@ envoy_cc_library(

envoy_cc_library(
name = "quic_platform_base_impl_lib",
srcs = ["quic_thread_impl.cc"],
hdrs = [
"quic_aligned_impl.h",
"quic_arraysize_impl.h",
Expand Down Expand Up @@ -124,7 +126,7 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:byte_order_lib",
"//source/server:backtrace_lib",
]),
]) + envoy_cc_platform_dep("//source/exe:platform_impl_lib"),
)

envoy_cc_library(
Expand Down
24 changes: 24 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/quic_thread_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// NOLINT(namespace-envoy)
//
// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_thread_impl.h"

#include <string>

#include "exe/platform_impl.h"

Envoy::Thread::ThreadFactory* getThreadFactory() {
static Envoy::PlatformImpl* platform_impl = new Envoy::PlatformImpl();

@mattklein123 mattklein123 Apr 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cc @jmarantz I haven't been tracking the entire convo here, but don't we want to avoid potentially having multiple platform instances in the process? I thought we were considering some type of init() function that the QUICHE platform layer could use to then store static references if needed?

@jmarantz jmarantz Apr 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is test-only code, right? Can it be moved to test/extensions/quic_listeners/quiche/platform along with all the other code that depends on it?

Then you can just use Thread::threadFactoryForTest() which is defined in test/test_common/thread_factory_for_test.h

@danzh2010 danzh2010 Apr 24, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I moved it to /test.

@danzh2010 danzh2010 Apr 24, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cc @jmarantz I haven't been tracking the entire convo here, but don't we want to avoid potentially having multiple platform instances in the process? I thought we were considering some type of init() function that the QUICHE platform layer could use to then store static references if needed?

Where to inject such init() function then? QUICHE has its own self-sufficient tests. The only place I could think of is overload testing::main, but not sure how much plumbing it would require.

@jmarantz jmarantz Apr 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My thinking after that last discussion on #6658 was that no init() is necessary as Quic only needs singleton access to the ThreadFactory in tests, and we already have a mechanism for that.

@mattklein123 mattklein123 Apr 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it's just tests, we can do what @jmarantz proposes. If it's in production code, I think an initialize function called from main after the platform is ready should be fine?

return &platform_impl->threadFactory();
}

namespace quic {

QuicThreadImpl::QuicThreadImpl(const std::string& /*name*/) {
thread_factory_ = getThreadFactory();
}

} // namespace quic
10 changes: 1 addition & 9 deletions 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 @@ -19,7 +19,7 @@ namespace quic {
// A class representing a thread of execution in QUIC.
class QuicThreadImpl {
public:
QuicThreadImpl(const std::string& /*name*/) {}
QuicThreadImpl(const std::string& name);
QuicThreadImpl(const QuicThreadImpl&) = delete;
QuicThreadImpl& operator=(const QuicThreadImpl&) = delete;

Expand Down Expand Up @@ -49,14 +49,6 @@ 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) {
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 Down
3 changes: 1 addition & 2 deletions 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,7 +3,6 @@ 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 @@ -41,7 +40,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: 5 additions & 11 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,8 +12,6 @@
#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 @@ -254,14 +252,10 @@ TEST_F(QuicPlatformTest, QuicStringPiece) {
}

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

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

~AdderThread() override = default;

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

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

// A QuicThread that is started and joined as usual.
{
AdderThread t1(&value, 1, platform_impl.threadFactory());
AdderThread t1(&value, 1);
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, platform_impl.threadFactory()).Start(); },
EXPECT_DEATH_LOG_TO_STDERR({ AdderThread(&value, 2).Start(); },
"QuicThread should be joined before destruction");
}

Expand Down