quiche: update QUICHE to commit 5dd7a030209f9a6b5043bebd8ac3ee54f18d1d08 by danzh2010 · Pull Request #17328 · envoyproxy/envoy

danzh2010 · 2021-07-14T03:18:45Z

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

Commit Message: Modify QUIC_BUG implementation to log with rate limit like ENVOY_BUG in release mode. Fix a QuicMemSliceSpanImpl life time issue which was exposed by new QUICHE change

Risk Level: low
Testing: added unit tests for QUIC_BUG and QuicMemSliceSpanImpl

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

repokitteh-read-only · 2021-07-14T03:18:50Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #17328 was opened by danzh2010.

see: more, trace.

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

moderation · 2021-07-14T05:32:20Z

You have some CI issues but...

/lgtm deps

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

danzh2010 · 2021-07-14T14:29:57Z

/assign @wu-bin

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

danzh2010 · 2021-07-15T17:27:58Z

It's ready for review now. PTAL!
Coverage test failure is caused by ProxyProtoFilterChainMatchIntegrationTest.MismatchDirectSourceButMatchSource/IPv4 which is irrelevant to this PR.

wu-bin · 2021-07-15T19:02:44Z

source/common/quic/platform/quic_mem_slice_span_impl.h

  Envoy::Buffer::Instance* buffer_{nullptr};
+  // Nullptr if this span is not constructed with a QuicMemSlice.
+  QuicMemSliceImpl* mem_slice_{nullptr};


Can you add some comments to explain the relationship of buffer_ and mem_slice_? (It seems we only need one of them to be !nullptr at a time, which is easier to understand)

done. buffer_ always points to the actually memory, but mem_slice_ is only used in the constructor which takes QuicMemSlice.

wu-bin · 2021-07-15T19:10:03Z

source/common/quic/platform/quiche_bug_tracker_impl.h

+  const std::string bug_name_;
+};
+
+class ScopedDisableExitOnQuicheBug {


nit: Clarify (in comments) it is test only, and only one instance can exists at a time.

done. But why only one instance can exists at a time?

I don't see comment added?

Suppose two threads, each has a ScopedDisableExitOnQuicheBug instance, called S1 and S2, and S1 and S2's lifetime overlaps like:

T=0: S1 construct
T=1: S2 construct
T=2: S1 destruct
T=3: S2 destruct

The final value for ScopedDisableExitOnQuicheBug will be true.

Oh, you mean multiple thread. Add comment for real now.

Yes multi thread is the main problem. But that sequence is problematic in single thread too.

Still not seeing comments. LGTM assuming you'll add one:)

How can scoped objects have interleaved life time?

wu-bin · 2021-07-15T19:11:59Z

source/common/quic/platform/quiche_bug_tracker_impl.cc

+      abort();
+    }
+#else
+    Envoy::Assert::invokeEnvoyBugFailureRecordActionForEnvoyBugMacroUseOnly(bug_name_.data());


nit: remove .data()

invokeEnvoyBugFailureRecordActionForEnvoyBugMacroUseOnly() take a char*

wu-bin · 2021-07-15T19:28:08Z

test/common/quic/platform/quic_platform_test.cc

  EXPECT_QUIC_BUG(bug("bug one is expected"), "bug one");
  EXPECT_QUIC_BUG(bug("bug two is expected"), "bug two");
+#ifdef NDEBUG
+  // The 3rd triggering in release mode should be omitted.


"should not be logged". If there's a bug counter it should still be incremented.

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

wu-bin

Thanks Dan. LGTM but please add a comment for ScopedDisableExitOnQuicheBug.

wu-bin · 2021-07-15T22:00:09Z

source/common/quic/platform/quiche_bug_tracker_impl.h

+  const std::string bug_name_;
+};
+
+class ScopedDisableExitOnQuicheBug {


Yes multi thread is the main problem. But that sequence is problematic in single thread too.

Still not seeing comments. LGTM assuming you'll add one:)

alyssawilk · 2021-07-19T13:50:43Z

(tagging as waiting on a new hash per internal discussion)

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

danzh2010 · 2021-07-19T18:14:19Z

(tagging as waiting on a new hash per internal discussion)

Updated to a newer commit. PTAL

moderation · 2021-07-19T22:01:34Z

bazel/repository_locations.bzl

        urls = ["https://storage.googleapis.com/quiche-envoy-integration/{version}.tar.gz"],
        use_category = ["dataplane_core"],
-        release_date = "2021-06-02",
+        release_date = "2021-07-13",


I think the date for this commit is 2021-07-16 as per https://quiche.googlesource.com/quiche/+/5dd7a030209f9a6b5043bebd8ac3ee54f18d1d08 but it's a little difficult to navigate googlesource.com. As this isn't a Github repo I don't think we check this programatically

moderation · 2021-07-19T22:02:16Z

/lgtm deps

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

alyssawilk

LGTM modulo outstanding comments

alyssawilk · 2021-07-20T15:20:59Z

@moderation I think this needs one more LGTM?

moderation · 2021-07-20T17:13:11Z

/lgtm deps

…d08 (envoyproxy#17328) Commit Message: Modify QUIC_BUG implementation to log with rate limit like ENVOY_BUG in release mode. Fix a QuicMemSliceSpanImpl life time issue which was exposed by new QUICHE change Risk Level: low Testing: added unit tests for QUIC_BUG and QuicMemSliceSpanImpl Signed-off-by: Dan Zhang <danzh@google.com>

danzh1989 added 3 commits July 13, 2021 22:03

update tar

5c1bd0b

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

remove unused impls

3d8600b

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

comment

fa9f785

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

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jul 14, 2021

typo

a3cbced

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

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jul 14, 2021

fix NOLINT

017c519

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

repokitteh-read-only bot assigned wu-bin Jul 14, 2021

danzh1989 added 5 commits July 14, 2021 11:07

use absl::string_view

5745a22

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

format

0f4f359

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

remove quic_containers_test

c10b75a

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

clang-tidy

606f96f

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

clang-tidy

478416a

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

wu-bin reviewed Jul 15, 2021

View reviewed changes

alyssawilk self-assigned this Jul 15, 2021

danzh1989 added 2 commits July 15, 2021 16:45

comment

a1b4e70

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

more comment

f360770

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

wu-bin previously approved these changes Jul 15, 2021

View reviewed changes

danzh2010 dismissed wu-bin’s stale review via f360770 July 15, 2021 23:21

alyssawilk added the waiting label Jul 19, 2021

update deps

9f819b2

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

repokitteh-read-only bot added deps Approval required for changes to Envoy's external dependencies and removed waiting labels Jul 19, 2021

danzh2010 changed the title ~~quiche: update QUICHE to commit 664d69ff8ddab53aef1c2d757874e1c192052e0a~~ quiche: update QUICHE to commit 5dd7a030209f9a6b5043bebd8ac3ee54f18d1d08 Jul 19, 2021

moderation reviewed Jul 19, 2021

View reviewed changes

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jul 19, 2021

release date

80fd6e9

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

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jul 20, 2021

alyssawilk approved these changes Jul 20, 2021

View reviewed changes

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jul 20, 2021

alyssawilk merged commit a1c5a50 into envoyproxy:main Jul 20, 2021

Conversation

danzh2010 commented Jul 14, 2021

Uh oh!

repokitteh-read-only bot commented Jul 14, 2021

Uh oh!

moderation commented Jul 14, 2021

Uh oh!

danzh2010 commented Jul 14, 2021

Uh oh!

danzh2010 commented Jul 15, 2021

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jul 19, 2021

Uh oh!

danzh2010 commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moderation commented Jul 19, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jul 20, 2021

Uh oh!

moderation commented Jul 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danzh2010 commented Jul 19, 2021 •

edited

Loading