quiche: add epoll_server for testing by danzh2010 · Pull Request #6650 · envoyproxy/envoy

danzh2010 · 2019-04-18T22:17:43Z

Add platform implementation for epoll_server. Build epoll_server and test it.
Add more DCHECK related macros in quic_logging_impl.h which is used by simple_epoll_server.cc
Update tar ball to quiche commit: 3e188a56edbcc471799499958bfd7c05ec45b9e2
Plumb through envoy_cc_test to pass in copt.

Risk Level: low
Testing: bring in simple_epoll_server_test.cc
Part of #2557

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

danzh2010 · 2019-04-18T22:18:17Z

/assign @wu-bin

wu-bin · 2019-04-19T02:00:43Z

bazel/external/quiche.BUILD

+)
+
+envoy_cc_test(
+    name = "epoll_server_test",


This test is also defined in test/extensions/quic_listeners/quiche/platform/BUILD, should we delete one of them?

Sorry, the one is //test/ is used for debugging. I removed that one instead.

wu-bin · 2019-04-19T02:03:47Z

bazel/external/quiche.BUILD

+        "quiche/epoll_server/fake_simple_epoll_server.h",
+        "quiche/epoll_server/simple_epoll_server.h",
+    ],
+    copts = envoy_copts("@envoy") + ["-Wno-error=unused-parameter"],


Instead of ignoring it, can we fix the unused-parameter warning in epoll_server code? We don't need to change envoy_build_system.bzl that way.

QUICHE allows unused-parameter, so I think we will run into this warning in quic and h2 code as well. That's why I modify envoy_build_system.bzl. Suppressing this warning in Envoy seems more reasonable than adding this enforcement in QUICHE.

SGTM, please move "-Wno-error=unused-parameter" into a global list and use it in here and epoll_server_test.

In case you missed it, this comment still needs to be resolved. @danzh2010

Where to put the global list? envoy_build_system.bzl?

In this file(bazel/external/quiche.BUILD), since it's only needed by quiche build rules.

done, and change it to -Wno-unused-parameter to suppress the warning entirely. Otherwise there will be too many warning output.

wu-bin · 2019-04-19T02:18:02Z

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

+
+int addressFamilyUnderTestHelper() {
+  std::vector<Envoy::Network::Address::IpVersion> versions =
+      Envoy::TestEnvironment::getIpVersionsForTest();


How about using std::find for clarity?

if (std::find(versions.begin(), versions.end(), Envoy::Network::Address::IpVersion::v4) {
return AF_INET;
}

if (std::find(versions.begin(), versions.end(), Envoy::Network::Address::IpVersion::v6) {
return AF_INET6;
}

return -1;

hmm, my code in comment doesn't actually work, it should be

if (std::find(versions.begin(), versions.end(), Envoy::Network::Address::IpVersion::v4) != versions.end()) {
return AF_INET;
}
// Same for v6

wu-bin · 2019-04-19T02:20:13Z

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

+} // namespace
+
+int AddressFamilyUnderTestImpl() {
+  static const int* version = new int(addressFamilyUnderTestHelper());


nit: no need to call new since int is trivial to destruct, just do:

static const int version = addressFamilyUnderTestHelper();
return version;

wu-bin · 2019-04-19T02:23:47Z

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

+#define EPOLL_PLOG_IMPL(severity) QUIC_PLOG_IMPL(severity)
+
+#ifndef NDEBUG
+#define EPOLL_DVLOG_IMPL(verbosity) QUIC_VLOG_IMPL(verbosity)


Why not
#define EPOLL_DVLOG_IMPL(verbosity) QUIC_DVLOG_IMPL(verbosity)
?

good point. done!

wu-bin · 2019-04-19T02:29:27Z

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

+
+namespace epoll_server {
+
+template <typename T, typename... Args> std::unique_ptr<T> EpollMakeUniqueImpl(Args&&... args) {


nit:

template <typename T, typename... Args>
using EpollMakeUniqueImpl = std::make_unique<T, Args...>;

alias doesn't work with function template.

Sorry about the wrong suggestion, and thanks for trying:)

wu-bin · 2019-04-19T02:34:00Z

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

+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#define EpollTestImpl ::testing::Test


nit: prefer type alias to macro:

namespace epoll_server {
using EpollTestImpl = ::testing::Test;
} // namespace epoll_server

wu-bin · 2019-04-19T02:38:32Z

source/extensions/quic_listeners/quiche/platform/quic_logging_impl.h

 #ifdef NDEBUG
 // Release build
-#define DCHECK(condition) QUIC_COMPILED_OUT_LOG()
+#define DCHECK(condition)                                                                          \


I'm curious, what is the problem without this change?

if we have:
{
bool a = true;
DCHECK(a)
}

In release build, w/o this change a is not used. We can supress unused-variable too. But that seems to be over-kill as QUICHE also forbids unused-variable.

Got it. There are other macros defined to "QUIC_COMPILED_OUT_LOG" as well, maybe change "QUIC_COMPILED_OUT_LOG" to take a "condition" instead?

#define QUIC_COMPILED_OUT_LOG(condition) QUIC_LOG_IMPL_INTERNAL(false && (condition), quic::NullLogStream().stream())

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

wu-bin · 2019-04-19T21:31:29Z

source/extensions/quic_listeners/quiche/platform/quic_logging_impl.h

-#define QUIC_DLOG_IF_IMPL(severity, condition) QUIC_COMPILED_OUT_LOG()
+#define DCHECK(condition) QUIC_COMPILED_OUT_LOG(condition)
+#define QUIC_COMPILED_OUT_LOG(condition)                                                           \
+  QUIC_LOG_IMPL_INTERNAL(false && condition, quic::NullLogStream().stream())


Please add parentheses around "condition": false && (condition)

Otherwise DCHECK(false || true) will unexpectedly create a NullLogStream.

wu-bin · 2019-04-19T21:35:07Z

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

+  std::vector<Envoy::Network::Address::IpVersion> versions =
+      Envoy::TestEnvironment::getIpVersionsForTest();
+  if (std::find(versions.begin(), versions.end(), Envoy::Network::Address::IpVersion::v4) !=
+      std::end(versions)) {


nit: How about we either use versions.(begin|end), or std::(begin|end)(versions), but not a mix of them?

wu-bin · 2019-04-19T21:37:27Z

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

+
+namespace epoll_server {
+
+template <typename T, typename... Args> std::unique_ptr<T> EpollMakeUniqueImpl(Args&&... args) {


Sorry about the wrong suggestion, and thanks for trying:)

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

wu-bin

LGTM, modulo the comment on "unused-parameter".

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

danzh2010 · 2019-04-22T15:40:06Z

/assign @alyssawilk

danzh2010 · 2019-04-22T15:42:55Z

/assign @htuch for changes in envoy_build_system.bzl

repokitteh-read-only · 2019-04-22T15:42:58Z

🙀 Error while processing event:

evaluation error
error: function _rk_error error: path contains forbidden characters

🐱

Caused by: a #6650 (comment) was created by @danzh2010.

see: more, trace.

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

alyssawilk · 2019-04-22T17:14:01Z

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

+
+} // namespace
+
+int AddressFamilyUnderTestImpl() {


Looks like this is confusingly named, but doing the right thing.
Possibly worth a rename or comments that this returns the "highest" supported address version of v4/v6. Given it's test code and I suspect will be unused I'm fine landing as-is.

Renaming requires upstream change. I added a comment here for the return value.

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

danzh2010 · 2019-04-22T19:36:19Z

/assign @htuch
For changes in envoy_build_system.bzl

htuch · 2019-04-23T16:21:19Z

bazel/external/quiche.BUILD

+    deps = ["@envoy//test/extensions/quic_listeners/quiche/platform:epoll_server_platform_impl_lib"],
+)
+
+cc_library(


Why are these cc_library rather than envoy_cc_library? The latter is preferred, it simplifies Google import and removes boilerplate.

I think the original intention was that these targets are not in the envoy workspace and to use envoy_cc_library we need to explicitly specify repository = "@envoy". But since it benefits to code import, I just switched to use envoy_cc_library.

I guess internally we will use a different QUICHE binding actually. But, it's good either way.

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

danzh2010 · 2019-04-23T19:24:21Z

/retest

repokitteh-read-only · 2019-04-23T19:24:25Z

🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #6650 (comment) was created by @danzh2010.

see: more, trace.

htuch

Looks good, one comment and needs master merge to resolve conflict.

htuch · 2019-04-23T20:18:29Z

bazel/external/quiche.BUILD

+        "quiche/epoll_server/fake_simple_epoll_server.h",
+        "quiche/epoll_server/simple_epoll_server.h",
+    ],
+    copts = envoy_copts("@envoy") + quiche_copt,


Do you need to explicitly add the envoy_copts if this is a envoy_cc_test_library?

Not needed actually. Removed.

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

htuch

LGTM, but needs master merge.

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

danzh2010 · 2019-04-26T16:49:07Z

sync'ed with #6700 to fix thread test failure. PTAL

htuch

Thanks!

* master: (35 commits) Revert "api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (envoyproxy#6692)" (envoyproxy#6761) Add test for the SocketOptionFactory::buildLiteralOptions() method. (envoyproxy#6724) Add test of parsing weighted_cluster route configuration to improve test coverage. (envoyproxy#6711) test: reducing H2 test permutations, increasing coverage time (envoyproxy#6753) Support gRPC-JSON translate without the google.api.http option. (envoyproxy#6731) quiche: implement QuicEpollClock (envoyproxy#6745) http: rc details for main Envoy workflow (envoyproxy#6560) quiche: implement QuicSystemEventLoopImpl (envoyproxy#6723) http: tracking 100s from upstream in stats (envoyproxy#6746) coverage: run without deprecated option (envoyproxy#6752) quiche: Implement spdy_test_helpers_impl. (envoyproxy#6741) [test] convert listener test stubs to v2 API (envoyproxy#6735) api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (envoyproxy#6692) quiche: Implement http2_reconstruct_object_impl.h. (envoyproxy#6717) build: patch protobuf for UBSAN issue. (envoyproxy#6721) router: scoped rds (2a): scoped routing configuration protos (envoyproxy#6675) tap: use move semantics for submitTrace (envoyproxy#6709) quiche: add epoll_server for testing (envoyproxy#6650) Increase timeout of the coverage test run to 3000 seconds as it is now bumping in the current 2000s limit causing coverage run to abort sometimes. (envoyproxy#6722) quiche: Update tarball to commit 43a1c0f10f2855c3cd142f500e8d19ac6d6f5a8c (envoyproxy#6718) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Add platform implementation for epoll_server. Build epoll_server and test it. Add more DCHECK related macros in quic_logging_impl.h which is used by simple_epoll_server.cc Update tar ball to quiche commit: 3e188a56edbcc471799499958bfd7c05ec45b9e2 Plumb through envoy_cc_test to pass in copt. Risk Level: low Testing: bring in simple_epoll_server_test.cc Part of envoyproxy#2557 Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Jeff Piazza <jeffpiazza@google.com>

danzh1989 added 2 commits April 18, 2019 18:07

Bring in epoll_server.

1068b8e

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

Merge branch 'master' into epoll_server

30a1dd6

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

danzh2010 requested review from alyssawilk and mattklein123 as code owners April 18, 2019 22:17

repokitteh-read-only bot assigned wu-bin Apr 18, 2019

wu-bin reviewed Apr 19, 2019

View reviewed changes

danzh1989 added 4 commits April 19, 2019 13:17

address comment

79570f3

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

fix build error

fa174ac

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

format

f44663a

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

improve macros

35c09c6

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

wu-bin reviewed Apr 19, 2019

View reviewed changes

enhance COMPILED_OUT_LOG

8105de3

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

wu-bin previously approved these changes Apr 19, 2019

View reviewed changes

move around unused-parameter

4c18507

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

danzh2010 dismissed wu-bin’s stale review via 4c18507 April 22, 2019 15:39

repokitteh-read-only bot assigned alyssawilk Apr 22, 2019

format

05fb83b

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

alyssawilk previously approved these changes Apr 22, 2019

View reviewed changes

add comment

2a6b9f8

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

danzh2010 dismissed alyssawilk’s stale review via 2a6b9f8 April 22, 2019 18:27

repokitteh-read-only bot assigned htuch Apr 22, 2019

htuch reviewed Apr 23, 2019

View reviewed changes

switch to envoy_cc_library

ea66710

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

htuch reviewed Apr 23, 2019

View reviewed changes

danzh1989 added 2 commits April 23, 2019 18:09

Merge branch 'master' into epoll_server

93f08e6

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

remove envoy_copts

751b7a9

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

htuch previously approved these changes Apr 23, 2019

View reviewed changes

Merge branch 'master' into epoll_server

3883e6d

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

htuch added the waiting label Apr 24, 2019

danzh2010 mentioned this pull request Apr 25, 2019

quiche:remove setThreadFactory() from QuicThreadImpl #6700

Merged

Merge branch 'master' into epoll_server

499ccb1

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

danzh2010 dismissed htuch’s stale review via 499ccb1 April 26, 2019 16:47

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

danzh2010 mentioned this pull request Apr 26, 2019

quiche: Implement http2_reconstruct_object_impl.h. #6717

Merged

htuch approved these changes Apr 26, 2019

View reviewed changes

htuch merged commit 3c367db into envoyproxy:master Apr 26, 2019


		namespace epoll_server {

		template <typename T, typename... Args> std::unique_ptr<T> EpollMakeUniqueImpl(Args&&... args) {

Conversation

danzh2010 commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Apr 18, 2019

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!

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!

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!

wu-bin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Apr 22, 2019

Uh oh!

danzh2010 commented Apr 22, 2019

Uh oh!

repokitteh-read-only bot commented Apr 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Apr 18, 2019 •

edited

Loading

wu-bin left a comment •

edited

Loading