grpc: utilities for inter-converting grpc::ByteBuffer and Buffer::Instance. by jplevyak · Pull Request #6732 · envoyproxy/envoy

jplevyak · 2019-04-27T17:55:43Z

Signed-off-by: John Plevyak jplevyak@gmail.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Add utilities for converting between grpc::ByteBuffer and Buffer::Instance and
for adding gRPC frame headers to Buffer::Instances. This is in support for proving raw
(Buffer::Instance) gRPC support which is in support of #4272. This PR also renames Grpc::Common::serializeBody
to Grpc::Common::serializeToGrpcFrame which is a better description of the actual behavior
as per comments on #6525,
Risk Level: low
Testing: Complete unit tests provided.
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

…tance and for adding gRPC frame headers to Buffer::Instances. Signed-off-by: John Plevyak <jplevyak@gmail.com>

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak · 2019-04-29T20:42:40Z

/retest

repokitteh-read-only · 2019-04-29T20:42:44Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6732 (comment) was created by @jplevyak.

see: more, trace.

lizan

Great, a couple of style nits and some questions.

source/common/grpc/common.cc

test/common/grpc/common_test.cc

source/common/grpc/common.cc

source/common/grpc/common.h

htuch

Looks good in general, a bunch of comments, mostly nits but a few thread related ones as well.
/wait

source/common/grpc/BUILD

source/common/grpc/common.cc

test/common/grpc/common_test.cc

source/common/grpc/common.cc

htuch · 2019-04-30T15:07:23Z

source/common/grpc/common.cc

+          delete container;
+        }
+      };
+  // NB: addBufferFragment takes a pointer alias to the BufferFragmentImpl which is passed in so we


Can we just make fragments_ a unique_ptr and have it follow RAII?

If I make it container->fragments_ = std::make_uniqueBuffer::BufferFragmentImpl[](slices.size());
Then I get an error because BufferFragmentImpl is not copyable:

unique_ptr.h:831:34: error: no matching constructor for initialization of 'remove_extent_t<Envoy::Buffer::BufferFragmentImpl []> []'

buffer_impl.h:430:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 0 were provided

Could you suggest an alternative?

Can BufferFragmentImpl be made copyable?

We could, but it was explicitly made NonCopyable because it is aliased by OwnedImpl, and therefore copying it could break the pointer from OwnedImpl, so I understand why it was made NonCopyable and I believe it needs to stay that way.

Any other ideas?

In response to: "Can you elaborate?":

The BufferFragmentImpl is not owned by the OwnedImpl, instead it takes a reference to it, so allowing the BufferFragmentImpl to move could result in hard to find bugs if someone erroneously thought that implied that it was a value type as opposed to what it is: something with strong address-based identity.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

source/common/grpc/common.cc

Signed-off-by: John Plevyak <jplevyak@gmail.com>

…-grpc-common Signed-off-by: John Plevyak <jplevyak@gmail.com>

lizan

Mostly style nits.

source/common/grpc/common.cc

source/common/grpc/google_grpc_utils.cc

test/common/grpc/common_test.cc

test/common/grpc/google_grpc_utils_test.cc

test/test_common/environment.cc

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak · 2019-05-07T20:12:45Z

/retest

repokitteh-read-only · 2019-05-07T20:12:50Z

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

🐱

Caused by: a #6732 (comment) was created by @jplevyak.

see: more, trace.

jplevyak · 2019-05-07T22:17:37Z

OK, the tests now pass (except for the asan which I believe is unrelated).

PTAL

lizan · 2019-05-07T23:22:30Z

also, merging master would fix asan.

source/common/grpc/BUILD

source/common/grpc/common.h

source/common/grpc/google_grpc_utils.cc

source/common/grpc/google_grpc_utils.h

test/common/grpc/google_grpc_utils_test.cc

source/common/grpc/google_grpc_utils.cc

Signed-off-by: John Plevyak <jplevyak@gmail.com>

…-grpc-common Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak · 2019-05-09T02:05:41Z

also, merging master would fix asan.

Done.

htuch · 2019-05-09T14:44:43Z

We could, but it was explicitly made NonCopyable because it is aliased by OwnedImpl, and therefore copying it could break the pointer from OwnedImpl, so I understand why it was made NonCopyable and I believe it needs to stay that way.

Re: this comment, I'm not sure I grok the "aliased by OwnedImpl" bit. Can you elaborate?

source/common/grpc/google_grpc_utils.cc

Signed-off-by: John Plevyak <jplevyak@gmail.com>

htuch

Great, thanks, appreciate your patience working through these issues.

* master: (88 commits) upstream: Null-deref on TCP health checker if setsockopt fails (envoyproxy#6793) ci: switch macOS CI to azure pipelines (envoyproxy#6889) os syscalls lib: break apart syscalls used for hot restart (envoyproxy#6880) Kafka codec: precompute request size before serialization, so we do n… (envoyproxy#6862) upstream: move static and strict_dns clusters to dedicated files (envoyproxy#6886) Rollforward of api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (envoyproxy#6692) (envoyproxy#6784) fix explicit constructor in copy-initialization (envoyproxy#6884) stats: use tag iterator rather than constructing the tag-array and searching that. (envoyproxy#6853) common: use unscoped build target in generate_version_linkstamp (envoyproxy#6877) Addendum to envoyproxy#6778 (envoyproxy#6882) ci: add minimum Linux build for Azure Pipelines (envoyproxy#6881) grpc: utilities for inter-converting grpc::ByteBuffer and Buffer::Instance. (envoyproxy#6732) upstream: allow excluding hosts from lb calculations until initial health check (envoyproxy#6794) stats: prevent unused counters from leaking across hot restart (envoyproxy#6850) network filters: add `injectDataToFilterChain(data, end_stream)` method to network filter callbacks (envoyproxy#6750) delete things that snuck back in (envoyproxy#6873) config: scoped rds (2b): support delta APIs in ConfigProvider framework (envoyproxy#6781) string == string! (envoyproxy#6868) config: add mssing imports to delta_subscription_state (envoyproxy#6869) protobuf: add missing default case to enum (envoyproxy#6870) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Add utilities for converting between grpc::ByteBuffer and Buffer::Ins…

42d4dcb

…tance and for adding gRPC frame headers to Buffer::Instances. Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak mentioned this pull request Apr 27, 2019

grpc: add a raw (Buffer::Instance) interface #6525

Merged

htuch self-assigned this Apr 29, 2019

htuch requested a review from lizan April 29, 2019 14:50

htuch assigned lizan Apr 29, 2019

jplevyak added 2 commits April 29, 2019 09:01

Fix CI test: use-after-move.

6014cd4

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Fix typo.

88ef0fc

Signed-off-by: John Plevyak <jplevyak@gmail.com>

lizan suggested changes Apr 30, 2019

View reviewed changes

htuch suggested changes Apr 30, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 30, 2019

Address comments.

253b584

Signed-off-by: John Plevyak <jplevyak@gmail.com>

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

Address comments.

5e0763a

Signed-off-by: John Plevyak <jplevyak@gmail.com>

htuch reviewed May 1, 2019

View reviewed changes

source/common/grpc/common.cc Outdated Show resolved Hide resolved

htuch reviewed May 1, 2019

View reviewed changes

source/common/grpc/common.cc Outdated Show resolved Hide resolved

jplevyak added 5 commits May 6, 2019 11:39

Address comments.

c54b9da

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Address alignment comment.

2860281

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Address additional comments.

542ea54

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Move google grpc specific code under compile options.

887f759

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into raw…

5f941b4

…-grpc-common Signed-off-by: John Plevyak <jplevyak@gmail.com>

lizan suggested changes May 6, 2019

View reviewed changes

jplevyak added 3 commits May 6, 2019 19:34

Address comments.

84e9709

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Fix release build.

deb27a3

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Do not ignore the return value of posix_memalign.

8f398ef

Signed-off-by: John Plevyak <jplevyak@gmail.com>

htuch suggested changes May 8, 2019

View reviewed changes

htuch reviewed May 8, 2019

View reviewed changes

source/common/grpc/google_grpc_utils.cc Show resolved Hide resolved

jplevyak added 5 commits May 8, 2019 17:20

Remove unnecessary dependency.

406bd50

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Address comments by adding comments.

377de24

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Address comment.

8a9b7f4

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Address comments.

70ecea2

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into raw…

5d716ae

…-grpc-common Signed-off-by: John Plevyak <jplevyak@gmail.com>

lizan previously approved these changes May 9, 2019

View reviewed changes

htuch mentioned this pull request May 9, 2019

quiche: implement QuicMemSlice #6400

Merged

htuch reviewed May 9, 2019

View reviewed changes

source/common/grpc/google_grpc_utils.cc Outdated Show resolved Hide resolved

htuch added the waiting label May 9, 2019

Address comments.

1c87b72

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak dismissed lizan’s stale review via 1c87b72 May 9, 2019 17:37

repokitteh-read-only bot removed the waiting label May 9, 2019

htuch approved these changes May 10, 2019

View reviewed changes

htuch merged commit d18b461 into envoyproxy:master May 10, 2019

Conversation

jplevyak commented Apr 27, 2019

Uh oh!

jplevyak commented Apr 29, 2019

Uh oh!

repokitteh-read-only bot commented Apr 29, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

jplevyak Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

htuch May 1, 2019

Choose a reason for hiding this comment

Uh oh!

jplevyak May 6, 2019

Choose a reason for hiding this comment

Uh oh!

jplevyak May 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jplevyak commented May 7, 2019

Uh oh!

repokitteh-read-only bot commented May 7, 2019

Uh oh!

jplevyak commented May 7, 2019

Uh oh!

lizan commented May 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jplevyak commented May 9, 2019

Uh oh!

htuch commented May 9, 2019