quiche:remove setThreadFactory() from QuicThreadImpl by danzh2010 · Pull Request #6700 · envoyproxy/envoy

danzh2010 · 2019-04-24T20:01:51Z

QuicThreadImpl is initialized within QUIC tests defined in QUICHE, it doesn't call upon setThreadFactory() added by #6658 while creating thread. This PR makes QuicThreadImpl itself to provide a 'singleton' Envoy::PlatformImpl instance so that the interface of QuicThreadImpl can be as desired by QUICHE.

Risk Level: low, not in use
Testing: covered by existing test

Part of #2557

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-04-24T20:04:14Z

/assign @mattklein123 @wu-bin
First trial to resolve discussion in #6658 (comment) to see if people like this approach.

wu-bin

Did you forget to commit quic_thread_impl.cc?

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-04-24T20:51:55Z

Did you forget to commit quic_thread_impl.cc?

Yes, sorry for missing it.

mattklein123

Thanks for the fix, 1 question:

/wait-any

mattklein123 · 2019-04-24T21:06:27Z

source/extensions/quic_listeners/quiche/platform/quic_thread_impl.cc

+#include "exe/platform_impl.h"
+
+Envoy::Thread::ThreadFactory* getThreadFactory() {
+  static Envoy::PlatformImpl* platform_impl = new Envoy::PlatformImpl();


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?

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

Yes, I moved it to /test.

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.

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.

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?

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz

basically looks good modulo one nit.

jmarantz · 2019-04-24T23:37:37Z

test/extensions/quic_listeners/quiche/platform/quic_thread_impl.h

 public:
-  QuicThreadImpl(const std::string& /*name*/) {}
+  QuicThreadImpl(const std::string& /*name*/) {
+    thread_factory_ = &Envoy::Thread::threadFactoryForTest();


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

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

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

wu-bin · 2019-04-25T01:30:04Z

bazel/external/quiche.BUILD


 cc_library(
-    name = "quic_platform_port_utils",
+    name = "quic_platform_for_test",


This change is incompatible with my #6693. Can you add a new library instead?

To clarify: QuicThread is test-only, but quic_port_utils.h is used in production so we need to split them, right? SGTM.

quic_port_utils.h is also test-only, the problem is that I'm trying to use smaller libraries in #6693 and this PR is doing the opposite:)

Should we just make sure that goes in first then?

@jmarantz: Yes, I'd prefer for that to go in first:) I just assigned it to you:)

Split into small libraries. This PR is blocking #6650. I hope to check in this one ASAP.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-04-25T19:48:12Z

sync'ed with #6693. PTAL

mattklein123

Nice

one more change needed

Signed-off-by: Dan Zhang <danzh@google.com>

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Jeff Piazza <jeffpiazza@google.com>

danzh2010 requested review from alyssawilk and mattklein123 as code owners April 24, 2019 20:01

remove setThreadFactory() from QuicThreadImpl

5783a64

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 force-pushed the quicthread branch from 8bcd7fa to 5783a64 Compare April 24, 2019 20:02

repokitteh-read-only bot assigned mattklein123 and wu-bin Apr 24, 2019

wu-bin reviewed Apr 24, 2019

View reviewed changes

mattklein123 added the waiting label Apr 24, 2019

add quic_thread_impl.cc

27b172c

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Apr 24, 2019

mattklein123 reviewed Apr 24, 2019

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Apr 24, 2019

danzh1989 added 3 commits April 24, 2019 18:31

move thread_impl to test

b2ee1d6

Signed-off-by: Dan Zhang <danzh@google.com>

remove thread_impl.cc

4394e37

Signed-off-by: Dan Zhang <danzh@google.com>

remove envoy_cc_platform_dep

1363a67

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz previously approved these changes Apr 24, 2019

View reviewed changes

wu-bin reviewed Apr 25, 2019

View reviewed changes

mattklein123 added the waiting label Apr 25, 2019

address comment

7e883e3

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed jmarantz’s stale review via 7e883e3 April 25, 2019 15:42

repokitteh-read-only bot removed the waiting label Apr 25, 2019

danzh1989 added 3 commits April 25, 2019 15:40

Merge branch 'master' into quicthread

c75da7e

Signed-off-by: Dan Zhang <danzh@google.com>

format

7ad9df4

Signed-off-by: Dan Zhang <danzh@google.com>

build file format

acb884d

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 changed the title ~~remove setThreadFactory() from QuicThreadImpl~~ quiche:remove setThreadFactory() from QuicThreadImpl Apr 25, 2019

mattklein123 previously approved these changes Apr 25, 2019

View reviewed changes

mattklein123 added the waiting label Apr 26, 2019

use reference

e4ae738

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Apr 26, 2019

mattklein123 approved these changes Apr 26, 2019

View reviewed changes

jmarantz approved these changes Apr 26, 2019

View reviewed changes

mattklein123 merged commit ca28063 into envoyproxy:master Apr 26, 2019

danzh2010 mentioned this pull request Apr 26, 2019

quiche: add epoll_server for testing #6650

Merged

jeffpiazza-google pushed a commit to jeffpiazza-google/envoy that referenced this pull request Apr 26, 2019

quiche:remove setThreadFactory() from QuicThreadImpl (envoyproxy#6700)

e077480

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Jeff Piazza <jeffpiazza@google.com>

Conversation

danzh2010 commented Apr 24, 2019

Uh oh!

danzh2010 commented Apr 24, 2019

Uh oh!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Apr 24, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Apr 25, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants