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 9 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
9 changes: 8 additions & 1 deletion bazel/external/quiche.BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,14 @@ cc_library(
deps = ["@envoy//test/extensions/quic_listeners/quiche/platform:quic_platform_test_output_impl_lib"],
)

cc_library(
name = "quic_platform_thread",
testonly = 1,
hdrs = ["quiche/quic/platform/api/quic_thread.h"],
visibility = ["//visibility:public"],
deps = ["@envoy//test/extensions/quic_listeners/quiche/platform:quic_platform_thread_impl_lib"],
)

cc_library(
name = "quic_platform_base",
hdrs = [
Expand Down Expand Up @@ -235,7 +243,6 @@ cc_library(
"quiche/quic/platform/api/quic_stack_trace.h",
"quiche/quic/platform/api/quic_string_utils.h",
"quiche/quic/platform/api/quic_text_utils.h",
"quiche/quic/platform/api/quic_thread.h",
],
"@envoy",
),
Expand Down
1 change: 1 addition & 0 deletions bazel/external/quiche.genrule_cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ cat <<EOF >sed_commands
/^#include/ s!net/quic/platform/impl/quic_port_utils_impl.h!test/extensions/quic_listeners/quiche/platform/quic_port_utils_impl.h!
/^#include/ s!net/quic/platform/impl/quic_test_impl.h!test/extensions/quic_listeners/quiche/platform/quic_test_impl.h!
/^#include/ s!net/quic/platform/impl/quic_test_output_impl.h!test/extensions/quic_listeners/quiche/platform/quic_test_output_impl.h!
/^#include/ s!net/quic/platform/impl/quic_thread_impl.h!test/extensions/quic_listeners/quiche/platform/quic_thread_impl.h!

# Rewrite include directives for platform impl files.
/^#include/ s!net/(http2|spdy|quic)/platform/impl/!extensions/quic_listeners/quiche/platform/!
Expand Down
2 changes: 0 additions & 2 deletions source/extensions/quic_listeners/quiche/platform/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ envoy_cc_library(
] + envoy_select_quiche([
"quic_logging_impl.h",
"quic_stack_trace_impl.h",
"quic_thread_impl.h",
]),
external_deps = [
"abseil_base",
Expand All @@ -117,7 +116,6 @@ envoy_cc_library(
"@com_googlesource_quiche//:quic_buffer_allocator_lib",
] + envoy_select_quiche([
":quic_platform_logging_impl_lib",
"//include/envoy/thread:thread_interface",
"//source/common/common:assert_lib",
"//source/common/common:byte_order_lib",
"//source/server:backtrace_lib",
Expand Down
14 changes: 12 additions & 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 @@ -44,7 +43,8 @@ envoy_cc_test(
"@com_googlesource_quiche//:quic_platform_port_utils",
"@com_googlesource_quiche//:quic_platform_sleep",
"@com_googlesource_quiche//:quic_platform_test_output",
] + envoy_cc_platform_dep("//source/exe:platform_impl_lib"),
"@com_googlesource_quiche//:quic_platform_thread",
],
)

envoy_cc_test(
Expand Down Expand Up @@ -82,6 +82,16 @@ envoy_cc_test_library(
],
)

envoy_cc_test_library(
name = "quic_platform_thread_impl_lib",
hdrs = ["quic_thread_impl.h"],
deps = [
"//include/envoy/thread:thread_interface",
"//source/common/common:assert_lib",
"//test/test_common:thread_factory_for_test_lib",
],
)

envoy_cc_test_library(
name = "quic_platform_test_impl_lib",
hdrs = ["quic_test_impl.h"],
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
15 changes: 6 additions & 9 deletions ...teners/quiche/platform/quic_thread_impl.h → ...teners/quiche/platform/quic_thread_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,19 @@

#include "common/common/assert.h"

#include "test/test_common/thread_factory_for_test.h"

#include "absl/synchronization/notification.h"

namespace quic {

// A class representing a thread of execution in QUIC.
class QuicThreadImpl {
public:
QuicThreadImpl(const std::string& /*name*/) {}
QuicThreadImpl(const std::string& /*name*/) {
thread_factory_ = &Envoy::Thread::threadFactoryForTest();

@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.

make thread_factory_ a reference and init in the initializer, per Envoy convention.

@danzh2010 danzh2010 Apr 25, 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.

done

@jmarantz jmarantz Apr 25, 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.

I refreshed a few times but this still doesn't look done to me; maybe this a github glitch?

@danzh2010 danzh2010 Apr 26, 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.

I made the change in the wrong branch. Now done in real.

}

QuicThreadImpl(const QuicThreadImpl&) = delete;
QuicThreadImpl& operator=(const QuicThreadImpl&) = delete;

Expand Down Expand Up @@ -49,14 +54,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