quiche: fix QuicMemSliceStorageImplTest and add quiche tests into CIs #10287

danzh2010 · 2020-03-06T17:15:05Z

QuicMemSliceStorageImpl is implemented with Envoy::Buffer::OwnedImpl which has a recent change in its move() implementation. The changed behavior is that move() will merge small slices into the last slice to avoid a lot of fragments. This breaks our assumption that move() between 2 buffers should keep the slices in their original form. As a result, it fails our QUICHE test QuicMemSliceStorageImplTest.MultipleIovInMultipleSlice in which we test building a QuicMemSliceStorage with max slice length 4 bytes.

[ RUN ] QuicMemSliceStorageImplTest.MultipleIovInMultipleSlice
bazel-out/k8-fastbuild/bin/external/com_googlesource_quiche/quiche/quic/platform/api/quic_mem_slice_storage_test.cc:56: Failure
Expected equality of these values:
"aaaa"
Which is: 0x3fd160
span.GetData(0)
Which is: "aaaabbbb"

Because the max slices length for this test is small enough for the Envoy buffer to merge them, QuicMemSliceStorage ends up doesn't honor the max_slice_len passed to its constructor. This could lead the slices in QuicStreamSendBuffer to become larger than QUIC expects.

This PR changes the implementation of QuicMemSliceStorageImpl and QuicMemSliceSpanImpl to work with this new Envoy Buffer behavior:
QuicMemSliceStorageImpl is changes to keep a vector of QuicMemSliceImpl instead of a Envoy buffer.
QuicMemSliceSpanImpl has a handle to a Envoy Buffer as before and the other handle to Span of QuicMemSliceImpl to work with the new implementation of QuicMemSliceStorageImpl.

Risk Level: low
Testing: existing tests pass

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

danzh2010 · 2020-03-06T17:16:08Z

/assign @wu-bin for platform impl change
/assign @lizan for do_ci.sh change

repokitteh-read-only · 2020-03-06T17:16:12Z

neither of for, platform, impl, change can be assigned to this issue.

🐱

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

see: more, trace.

repokitteh-read-only · 2020-03-06T17:16:14Z

🙀 Error while processing event:

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

🐱

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

see: more, trace.

danzh2010 · 2020-03-06T17:17:18Z

/assign @wu-bin @lizan

danzh2010 · 2020-03-06T17:19:06Z

/retest

repokitteh-read-only · 2020-03-06T17:19:10Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

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

see: more, trace.

jmarantz

basically looks good but I was hoping @antoniovicente could take a look as I think he's been thinking more about the buffers code.

jmarantz · 2020-03-06T22:49:57Z

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

+    if (buffer_ != nullptr) {
+      return buffer_->length();
+    } else {
+      size_t len = 0;


if we are returning a QuicByteCount rather than size_t should we use that for the accumulation?

good point!

jmarantz · 2020-03-06T22:51:12Z

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


 private:
+  // Either |buffer_| or |span_| is used to point to the mem slices.


line 69 above assumes that they can't both be populated, but it's a little ambiguous from this comment. Can you clarify in this comment?

wu-bin · 2020-03-09T11:39:04Z

ci/do_ci.sh

@@ -92,7 +92,7 @@ if [[ $# -gt 1 ]]; then
  shift
  TEST_TARGETS=$*
 else
-  TEST_TARGETS=//test/...
+  TEST_TARGETS="//test/... $(bazel query ${BAZEL_QUERY_OPTIONS} "attr('size', 'small', '@com_googlesource_quiche//:all')" | grep "^@com_googlesource_quiche")"


Why only run small quiche tests?

I changed to check a new tag 'test_included'. We want to run QUICHE tests selectively because we may add a test target quic_end_to_end_test which is can be ran manually in case of need.

wu-bin · 2020-03-09T11:56:59Z

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

+  if (buffer_ != nullptr) {
+    uint64_t num_slices = buffer_->getRawSlices(nullptr, 0);
+    ASSERT(num_slices > index);
+    absl::FixedArray<Envoy::Buffer::RawSlice> slices(num_slices);
+    buffer_->getRawSlices(slices.begin(), num_slices);
+    return {reinterpret_cast<char*>(slices[index].mem_), slices[index].len_};
+  }


Can we store a absl::FixedArray<Envoy::Buffer::RawSlice> as a member variable, and convert the slices in Envoy::Buffer::Instance and absl::Span<QuicMemSliceImpl> to the FixedArray? It seems to me that QuicMemSliceSpanImpl never owns a buffer, so making that change will result in a much smaller implementation.

We don't have a way to convert a RawSlice to QuicMemSliceImpl which is used in ConsumeFunction. RawSlice have no control over the memory but QuicMemSliceImpl does.

I'm confused, I think in ConsumeAll, we are already converting a RawSlice to a QuicMemSlice?

absl::FixedArray<Envoy::Buffer::RawSlice> slices(num_slices); buffer_->getRawSlices(slices.begin(), num_slices); for (auto& slice : slices) { if (slice.len_ == 0) { continue; } // Move each slice into a stand-alone buffer. // TODO(danzh): investigate the cost of allocating one buffer per slice. // If it turns out to be expensive, add a new function to free data in the middle in buffer // interface and re-design QuicMemSliceImpl. consume(QuicMemSlice(QuicMemSliceImpl(*buffer_, slice.len_))); saved_length += slice.len_; }

We can transfer the ownership of memory from Envoy Buffer to QuicMemSlice, but not from RawSlice to QuicMemSlice. My understanding is that QuicMemSliceSpan needs to have a handle to the owner of the memory.

wu-bin · 2020-03-09T12:10:12Z

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

+      for (auto& mem_slice : other.mem_slices_) {
+        Envoy::Buffer::OwnedImpl buffer;
+        buffer.add(mem_slice.data(), mem_slice.length());
+        mem_slices_.emplace_back(buffer, mem_slice.length());


I think making copies is fine since it is only needed in tests, but worth adding a comment.

This is copy assignment, making copy is expected. And yes, this is only used in QUICHE tests.

Fair enough.

wu-bin · 2020-03-09T12:11:48Z

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

+    quic::QuicUniqueBufferPtr buffer = quic::MakeUniqueBuffer(allocator, slice_len);
+    QuicUtils::CopyToBuffer(iov, iov_count, io_offset, slice_len, buffer.get());
+    mem_slices_.emplace_back(std::move(buffer), slice_len);


Looks much better than before. Thanks!

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

danzh2010 · 2020-03-11T20:06:56Z

ping?

danzh2010 · 2020-03-11T21:15:28Z

Discussed with @wu-bin offline. The current QuicMemSlice API is not very straight forward for user. And there might be better API for it together with QuicMemSliceSpan and QuicMemSliceStorage. We want to try the upstream improvement first before fixing this failure. So I will close this PR for now and resurrect it once we get some conclusion about the best approach.

danzh1989 added 5 commits March 5, 2020 16:02

fix mem slice storage construction

48e3890

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

add com_googlesource_quiche test

6b48b0a

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

Merge branch 'master' into fixmemslice

6a0238d

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

fix typo

f092feb

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

revert protobuf BUID

12753dc

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

danzh2010 requested review from alyssawilk and mattklein123 as code owners March 6, 2020 17:15

repokitteh-read-only bot assigned lizan and wu-bin Mar 6, 2020

mattklein123 assigned jmarantz Mar 6, 2020

jmarantz reviewed Mar 6, 2020

View reviewed changes

wu-bin reviewed Mar 9, 2020

View reviewed changes

danzh1989 added 6 commits March 9, 2020 14:23

fix coverage test

f5e52a3

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

comment and type

22e473c

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

format

19dd864

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

Merge branch 'master' into fixmemslice

a51c85d

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

format

99dd8d9

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

typo

cb9500f

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

danzh2010 requested review from lizan and PiotrSikora as code owners March 10, 2020 15:45

format

82d6d92

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

danzh2010 closed this Mar 11, 2020

lambdai mentioned this pull request Apr 20, 2020

release-v1.12: back port #9509 http: Fix ASSERT failure and infinite loop when attempting to unset readDisable state on a closed connection #10802

Closed

lizan mentioned this pull request May 8, 2020

backport: various build fixes for 1.13.x #11054

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quiche: fix QuicMemSliceStorageImplTest and add quiche tests into CIs #10287

quiche: fix QuicMemSliceStorageImplTest and add quiche tests into CIs #10287

danzh2010 commented Mar 6, 2020

danzh2010 commented Mar 6, 2020

repokitteh-read-only bot commented Mar 6, 2020

repokitteh-read-only bot commented Mar 6, 2020

danzh2010 commented Mar 6, 2020

danzh2010 commented Mar 6, 2020

repokitteh-read-only bot commented Mar 6, 2020

jmarantz left a comment

jmarantz Mar 6, 2020

danzh2010 Mar 9, 2020

jmarantz Mar 6, 2020

danzh2010 Mar 9, 2020

wu-bin Mar 9, 2020

danzh2010 Mar 9, 2020

wu-bin Mar 9, 2020

danzh2010 Mar 9, 2020

wu-bin Mar 10, 2020

danzh2010 Mar 10, 2020

wu-bin Mar 9, 2020

danzh2010 Mar 9, 2020

wu-bin Mar 10, 2020

wu-bin Mar 9, 2020

danzh2010 commented Mar 11, 2020

danzh2010 commented Mar 11, 2020


		private:
		// Either \|buffer_\| or \|span_\| is used to point to the mem slices.

quiche: fix QuicMemSliceStorageImplTest and add quiche tests into CIs #10287

quiche: fix QuicMemSliceStorageImplTest and add quiche tests into CIs #10287

Conversation

danzh2010 commented Mar 6, 2020

danzh2010 commented Mar 6, 2020

repokitteh-read-only bot commented Mar 6, 2020

repokitteh-read-only bot commented Mar 6, 2020

danzh2010 commented Mar 6, 2020

danzh2010 commented Mar 6, 2020

repokitteh-read-only bot commented Mar 6, 2020

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 commented Mar 11, 2020

danzh2010 commented Mar 11, 2020