ratelimit: convert to Grpc::AsyncClient. by htuch · Pull Request #1267 · envoyproxy/envoy

htuch · 2017-07-14T22:35:43Z

No description provided.

htuch · 2017-07-14T22:36:16Z

@mattklein123 This is WiP - I still need to add some more unit tests for coverage and the actual integration test that motivated this, but thought I would throw it out for comments early.

mattklein123

A few comments but at a high level LGTM.

mattklein123 · 2017-07-16T17:05:06Z

source/common/ratelimit/ratelimit.proto


 option go_package = "ratelimit";

-option cc_generic_services = true;


It might be out of scope for this change, but if possible can we pull this proto from envoy-api and delete this file also? There is a GH open on that.

Yeah, I think we want to cut to the v2 API when we switch to envoy-api, and there we have a different package namespace etc., so that's best left as future work.

mattklein123 · 2017-07-16T17:06:10Z

source/common/ratelimit/ratelimit_impl.cc

-  ASSERT(callbacks_);
-  channel_->cancel();
+  ASSERT(callbacks_ != nullptr);
+  ASSERT(stream_ != nullptr);


nit: will crash on the next line in obvious way

mattklein123 · 2017-07-16T17:07:51Z

source/common/ratelimit/ratelimit_impl.cc

-  callbacks_->complete(status);
-  callbacks_ = nullptr;
-}
+  LimitStatus limit_status = LimitStatus::OK;


Question: It seems kind of odd to do this logic in the context of onRemoteClose(). Why not onReceiveMessage() ? I guess the larger question is for unary APIs (which are very common) can we make coding against this easier vs. having to remember to close the stream, etc.?

Yep, this is a good idea, I was planning to provide a unary RPC interface that didn't require users to keep track of the minutiae of stream state before finalizing the PR.

… fixes.

htuch · 2017-07-17T22:57:55Z

@mattklein123 This now works and is ready for review. It's pretty big - if you are OK reviewing as is that's great, otherwise I can try and tease apart stuff into smaller PRs.

@msample I'm hoping this resolves your issue when using Grpc::AsyncClient in your custom filter.

mattklein123 · 2017-07-17T23:02:26Z

I'm working on fixing #1254 then I can review.

mattklein123

very nice. few comments.

mattklein123 · 2017-07-18T15:16:49Z

include/envoy/grpc/async_client.h

-        AsyncClientCallbacks<ResponseType>& callbacks,
-        const Optional<std::chrono::milliseconds>& timeout) PURE;
+  virtual AsyncStream<RequestType>* start(const Protobuf::MethodDescriptor& service_method,
+                                          AsyncStreamCallbacks<ResponseType>& callbacks,


Since we are now splitting streaming and unary, can we kill the timeout in the streaming case? IMO it doesn't make any sense. (I know the underlying AsyncClient stuff still has this problem, but we might as well fix it here?)

mattklein123 · 2017-07-18T15:18:24Z

source/common/grpc/async_client_impl.h


-    grpc_stream->set_stream(http_stream);
+    grpc_stream->moveIntoList(std::move(grpc_stream), active_streams_);
+    return dynamic_cast<AsyncRequestImpl<RequestType, ResponseType>*>(


perf nit: This has cost. Typically I would just hold a local reference/pointer to the unique_ptr, then return it after you add to the list.

mattklein123 · 2017-07-18T15:23:07Z

source/common/grpc/async_client_impl.h

-      LinkedObject<AsyncClientStreamImpl<RequestType, ResponseType>>::removeFromList(
-          parent_.active_streams_);
+    if (LinkedObject<AsyncStreamImpl<RequestType, ResponseType>>::inserted()) {
+      parent_.dispatcher_.deferredDelete(


You probably already looked at this, but just double check that the stream/request does not reference client in its destructor, or there might be ordering issues depending on how client is destructed. (I haven't looked at code but just pointing this out from past experience).

mattklein123 · 2017-07-18T15:24:23Z

source/common/http/async_client_impl.cc

  // the immediate failure case.
  if (inserted()) {
-    removeFromList(parent_.active_streams_);
+    dispatcher().deferredDelete(removeFromList(parent_.active_streams_));


Same comment here about referencing client in destructor.

mattklein123 · 2017-07-18T15:27:02Z

source/common/ratelimit/ratelimit_impl.h

+
+typedef Grpc::AsyncRequestCallbacks<pb::lyft::ratelimit::RateLimitResponse> RateLimitAsyncCallbacks;
+
+class GrpcClientImpl : public Client, public RateLimitAsyncCallbacks {


In looking at this code again, I realize that it's kind of dumb that we make a new "client" for every filter/request. Can you just drop a TODO in here while you are in here to optimize this at some point? (We should have thread local client per filter, and track outstanding requests).

…lient

mattklein123

nice

mattklein123 · 2017-07-18T21:55:11Z

source/common/grpc/async_client_impl.h


  AsyncStream<RequestType>* start(const Protobuf::MethodDescriptor& service_method,
                                  AsyncStreamCallbacks<ResponseType>& callbacks) override {
+    const Optional<std::chrono::milliseconds> no_timeout;


Why was this change needed to fix ASAN? This still feels kind of busted to me. Aren't we storing a reference to the timeout inside the AsyncStreamImpl? If so should this be at client scope?

I checked and it does get copied inside the underlying Http::AsyncClient::Stream, in the owned RouteEntryImpl https://github.com/lyft/envoy/blob/0efa18c36d6f789562690b5053c2c4b00987979e/source/common/http/async_client_impl.h#L123.

More generally, this got me wondering about whether we should have a better convention for handling reference ownership semantics in Envoy - it's hard to tell from the type signature of a method alone whether it will store a reference for later use or copy. I think there's a fair bit of mixed use of this in Envoy.

The Grpc::AsyncStreamImpl was not owning the timeout in period between object construction and passing to the Http::AsyncClient::Stream in initialize(), which is what triggered the ASAN fail.

I see, it gets used in the context of initialize(). Sure open to suggestions on naming. Same problem applies to pointers or references. Not sure if there is a good naming scheme.

A first thought is FooBar& for the case when a reference is not retained past the unwinding of the call stack, FooBarRetainedRef for when it is. But, that's super verbose. @dnoe for thoughts as well.

. Turns out that a sendLocalReply() will unconditionally send a body after errors such as 504, gRPC client performs a reset before this and had self-destructed already.

Turns out that a sendLocalReply() will unconditionally send a body after errors such as 504, gRPC client performs a reset before this and had self-destructed already.

Automatic merge from submit-queue. Fix quota cache status assignment. **What this PR does / why we need it**:Fix a bug in quota amount check in MixerClientImpl::Check(). **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes envoyproxy#1181 **Special notes for your reviewer**: **Release note**: ```release-note NONE ```

ratelimit: convert to Grpc::AsyncClient.

27784ef

mattklein123 reviewed Jul 16, 2017

View reviewed changes

htuch added 2 commits July 16, 2017 22:33

Switch to new Grpc::AsyncRequest wrapper for unary RPCs.

cf50ad8

Ratelimit integration test, deferred delete and other gRPC client bug…

5d24923

… fixes.

mattklein123 reviewed Jul 18, 2017

View reviewed changes

htuch added 7 commits July 18, 2017 12:34

Merge remote-tracking branch 'upstream/master' into ratelimit-async-c…

7d0c9e5

…lient

Remove timeout from gRPC AsyncStreams.

63ae182

Perf nit.

bbc1869

Fix format.

f0ccd08

TODO on per-thread gRPC client fo RateLimit.

0589233

Unit test for missing AsyncRequest code coverage.

7b078f8

Fix format.

a964f01

mattklein123 previously approved these changes Jul 18, 2017

View reviewed changes

htuch mentioned this pull request Jul 18, 2017

Deprecate RpcChannel for unary gRPC #1102

Closed

Fix object lifetime snafu that broke ASAN.

1470d55

htuch dismissed mattklein123’s stale review via 1470d55 July 18, 2017 21:35

mattklein123 reviewed Jul 18, 2017

View reviewed changes

mattklein123 approved these changes Jul 18, 2017

View reviewed changes

htuch merged commit 8554be4 into envoyproxy:master Jul 18, 2017

htuch deleted the ratelimit-async-client branch July 18, 2017 22:27


		option go_package = "ratelimit";

		option cc_generic_services = true;


		typedef Grpc::AsyncRequestCallbacks<pb::lyft::ratelimit::RateLimitResponse> RateLimitAsyncCallbacks;

		class GrpcClientImpl : public Client, public RateLimitAsyncCallbacks {

Conversation

htuch commented Jul 14, 2017

Uh oh!

htuch commented Jul 14, 2017

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!

htuch commented Jul 17, 2017

Uh oh!

mattklein123 commented Jul 17, 2017

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants