From 9117dbdd2b4baa2b45587e9ed0d1544a26c1f7c8 Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Thu, 12 Mar 2020 14:52:22 +0100 Subject: [PATCH 1/9] async client: introduce `AsyncClientRequestTracker` to keep track of inflight requests and cancel them on destruction Signed-off-by: Yaroslav Skopets --- source/common/http/BUILD | 9 ++++ source/common/http/async_client_utility.cc | 36 ++++++++++++++ source/common/http/async_client_utility.h | 32 ++++++++++++ test/common/http/BUILD | 9 ++++ test/common/http/async_client_utility_test.cc | 49 +++++++++++++++++++ 5 files changed, 135 insertions(+) create mode 100644 source/common/http/async_client_utility.cc create mode 100644 source/common/http/async_client_utility.h create mode 100644 test/common/http/async_client_utility_test.cc diff --git a/source/common/http/BUILD b/source/common/http/BUILD index e13f8a1fd16b8..165b250813acd 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -35,6 +35,15 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "async_client_utility_lib", + srcs = ["async_client_utility.cc"], + hdrs = ["async_client_utility.h"], + deps = [ + "//include/envoy/http:async_client_interface", + ], +) + envoy_cc_library( name = "codec_client_lib", srcs = ["codec_client.cc"], diff --git a/source/common/http/async_client_utility.cc b/source/common/http/async_client_utility.cc new file mode 100644 index 0000000000000..3233746d50815 --- /dev/null +++ b/source/common/http/async_client_utility.cc @@ -0,0 +1,36 @@ +#include "common/http/async_client_utility.h" + +namespace Envoy { +namespace Http { + +AsyncClientRequestTracker::~AsyncClientRequestTracker() { + for (auto* active_request : active_requests_) { + active_request->cancel(); + } +} + +AsyncClientRequestTracker& AsyncClientRequestTracker::operator+=(AsyncClient::Request* request) { + // Let client code to avoid conditionals. + if (request) { + ASSERT(active_requests_.find(request) == active_requests_.end()); + active_requests_.insert(request); + } + return *this; +} + +AsyncClientRequestTracker& +AsyncClientRequestTracker::operator-=(const AsyncClient::Request* request) { + // Let client code to avoid conditionals. + if (request) { + auto it = active_requests_.find(const_cast(request)); + // Support a use case where request callbacks might get called prior to a request handle + // is returned from AsyncClient::send(). + if (it != active_requests_.end()) { + active_requests_.erase(it); + } + } + return *this; +} + +} // namespace Http +} // namespace Envoy diff --git a/source/common/http/async_client_utility.h b/source/common/http/async_client_utility.h new file mode 100644 index 0000000000000..29d373daff1a9 --- /dev/null +++ b/source/common/http/async_client_utility.h @@ -0,0 +1,32 @@ +#pragma once + +#include "envoy/http/async_client.h" + +namespace Envoy { +namespace Http { + +/** + * Keeps track of active async HTTP requests to be able to cancel them on destruction. + */ +class AsyncClientRequestTracker { +public: + /** + * Cancels all known active async HTTP requests. + */ + ~AsyncClientRequestTracker(); + /** + * Includes a given async HTTP request into a set of known active requests. + */ + AsyncClientRequestTracker& operator+=(AsyncClient::Request* request); + /** + * Excludes a given async HTTP request from a set of known active requests. + */ + AsyncClientRequestTracker& operator-=(const AsyncClient::Request* request); + +private: + // Track active async HTTP requests to be able to cancel them on destruction. + std::unordered_set active_requests_; +}; + +} // namespace Http +} // namespace Envoy diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 593d22653e689..92360ccd4e3ee 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -36,6 +36,15 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "async_client_utility_test", + srcs = ["async_client_utility_test.cc"], + deps = [ + "//source/common/http:async_client_utility_lib", + "//test/mocks/http:http_mocks", + ], +) + envoy_cc_test( name = "codec_client_test", srcs = ["codec_client_test.cc"], diff --git a/test/common/http/async_client_utility_test.cc b/test/common/http/async_client_utility_test.cc new file mode 100644 index 0000000000000..d5618a10ca5f5 --- /dev/null +++ b/test/common/http/async_client_utility_test.cc @@ -0,0 +1,49 @@ +#include "common/http/async_client_utility.h" + +#include "test/mocks/http/mocks.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::NiceMock; +using testing::StrictMock; + +namespace Envoy { +namespace Http { +namespace { + +class AsyncClientRequestTrackerTest : public testing::Test { +public: + std::unique_ptr active_requests_{ + std::make_unique()}; + + NiceMock async_client_; + StrictMock request1_{&async_client_}; + StrictMock request2_{&async_client_}; + StrictMock request3_{&async_client_}; +}; + +TEST_F(AsyncClientRequestTrackerTest, OnDestructDoNothingIfThereAreNoActiveRequests) { + // Trigger destruction. + active_requests_.reset(); +} + +TEST_F(AsyncClientRequestTrackerTest, OnDestructCancelActiveRequests) { + // Include active requests. + *active_requests_ += &request1_; + *active_requests_ += &request2_; + *active_requests_ += &request3_; + // Exclude active requests. + *active_requests_ -= &request2_; + + // Must cancel active requests on destruction. + EXPECT_CALL(request1_, cancel()); + EXPECT_CALL(request3_, cancel()); + + // Trigger destruction. + active_requests_.reset(); +} + +} // namespace +} // namespace Http +} // namespace Envoy From 943a49938c65b5f03096e96b2e955c15c17c53ec Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Sun, 15 Mar 2020 10:29:24 +0100 Subject: [PATCH 2/9] Kick CI Signed-off-by: Yaroslav Skopets From 3136d4dbbc556cdbe0773f263de45d64e43d7985 Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Tue, 17 Mar 2020 00:28:14 +0100 Subject: [PATCH 3/9] code review: use regular methods instead of operator overloading Signed-off-by: Yaroslav Skopets --- source/common/http/async_client_utility.cc | 5 ++--- source/common/http/async_client_utility.h | 4 ++-- test/common/http/async_client_utility_test.cc | 8 ++++---- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/source/common/http/async_client_utility.cc b/source/common/http/async_client_utility.cc index 3233746d50815..5c0230f163b90 100644 --- a/source/common/http/async_client_utility.cc +++ b/source/common/http/async_client_utility.cc @@ -9,7 +9,7 @@ AsyncClientRequestTracker::~AsyncClientRequestTracker() { } } -AsyncClientRequestTracker& AsyncClientRequestTracker::operator+=(AsyncClient::Request* request) { +AsyncClientRequestTracker& AsyncClientRequestTracker::add(AsyncClient::Request* request) { // Let client code to avoid conditionals. if (request) { ASSERT(active_requests_.find(request) == active_requests_.end()); @@ -18,8 +18,7 @@ AsyncClientRequestTracker& AsyncClientRequestTracker::operator+=(AsyncClient::Re return *this; } -AsyncClientRequestTracker& -AsyncClientRequestTracker::operator-=(const AsyncClient::Request* request) { +AsyncClientRequestTracker& AsyncClientRequestTracker::remove(const AsyncClient::Request* request) { // Let client code to avoid conditionals. if (request) { auto it = active_requests_.find(const_cast(request)); diff --git a/source/common/http/async_client_utility.h b/source/common/http/async_client_utility.h index 29d373daff1a9..f93cc8a4a807d 100644 --- a/source/common/http/async_client_utility.h +++ b/source/common/http/async_client_utility.h @@ -17,11 +17,11 @@ class AsyncClientRequestTracker { /** * Includes a given async HTTP request into a set of known active requests. */ - AsyncClientRequestTracker& operator+=(AsyncClient::Request* request); + AsyncClientRequestTracker& add(AsyncClient::Request* request); /** * Excludes a given async HTTP request from a set of known active requests. */ - AsyncClientRequestTracker& operator-=(const AsyncClient::Request* request); + AsyncClientRequestTracker& remove(const AsyncClient::Request* request); private: // Track active async HTTP requests to be able to cancel them on destruction. diff --git a/test/common/http/async_client_utility_test.cc b/test/common/http/async_client_utility_test.cc index d5618a10ca5f5..ae78f3a074b28 100644 --- a/test/common/http/async_client_utility_test.cc +++ b/test/common/http/async_client_utility_test.cc @@ -30,11 +30,11 @@ TEST_F(AsyncClientRequestTrackerTest, OnDestructDoNothingIfThereAreNoActiveReque TEST_F(AsyncClientRequestTrackerTest, OnDestructCancelActiveRequests) { // Include active requests. - *active_requests_ += &request1_; - *active_requests_ += &request2_; - *active_requests_ += &request3_; + active_requests_->add(&request1_); + active_requests_->add(&request2_); + active_requests_->add(&request3_); // Exclude active requests. - *active_requests_ -= &request2_; + active_requests_->remove(&request2_); // Must cancel active requests on destruction. EXPECT_CALL(request1_, cancel()); From 6c986b39ef56ad92b5787cff1f93190cb64b5f4d Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Tue, 17 Mar 2020 00:30:45 +0100 Subject: [PATCH 4/9] code review: use `absl::flat_hash_set` instead of `std::unordered_set` Signed-off-by: Yaroslav Skopets --- source/common/http/async_client_utility.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/async_client_utility.h b/source/common/http/async_client_utility.h index f93cc8a4a807d..c4a22e4353ee9 100644 --- a/source/common/http/async_client_utility.h +++ b/source/common/http/async_client_utility.h @@ -25,7 +25,7 @@ class AsyncClientRequestTracker { private: // Track active async HTTP requests to be able to cancel them on destruction. - std::unordered_set active_requests_; + absl::flat_hash_set active_requests_; }; } // namespace Http From 1d04f5fded7740c6a041bbd662f6f16c73f9b729 Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Tue, 17 Mar 2020 19:24:46 +0100 Subject: [PATCH 5/9] code review: change return type to `void` Signed-off-by: Yaroslav Skopets --- source/common/http/async_client_utility.cc | 6 ++---- source/common/http/async_client_utility.h | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/source/common/http/async_client_utility.cc b/source/common/http/async_client_utility.cc index 5c0230f163b90..dbcab993e23ae 100644 --- a/source/common/http/async_client_utility.cc +++ b/source/common/http/async_client_utility.cc @@ -9,16 +9,15 @@ AsyncClientRequestTracker::~AsyncClientRequestTracker() { } } -AsyncClientRequestTracker& AsyncClientRequestTracker::add(AsyncClient::Request* request) { +void AsyncClientRequestTracker::add(AsyncClient::Request* request) { // Let client code to avoid conditionals. if (request) { ASSERT(active_requests_.find(request) == active_requests_.end()); active_requests_.insert(request); } - return *this; } -AsyncClientRequestTracker& AsyncClientRequestTracker::remove(const AsyncClient::Request* request) { +void AsyncClientRequestTracker::remove(const AsyncClient::Request* request) { // Let client code to avoid conditionals. if (request) { auto it = active_requests_.find(const_cast(request)); @@ -28,7 +27,6 @@ AsyncClientRequestTracker& AsyncClientRequestTracker::remove(const AsyncClient:: active_requests_.erase(it); } } - return *this; } } // namespace Http diff --git a/source/common/http/async_client_utility.h b/source/common/http/async_client_utility.h index c4a22e4353ee9..a4c836bf0a345 100644 --- a/source/common/http/async_client_utility.h +++ b/source/common/http/async_client_utility.h @@ -17,11 +17,11 @@ class AsyncClientRequestTracker { /** * Includes a given async HTTP request into a set of known active requests. */ - AsyncClientRequestTracker& add(AsyncClient::Request* request); + void add(AsyncClient::Request* request); /** * Excludes a given async HTTP request from a set of known active requests. */ - AsyncClientRequestTracker& remove(const AsyncClient::Request* request); + void remove(const AsyncClient::Request* request); private: // Track active async HTTP requests to be able to cancel them on destruction. From efeab01e3b33b5140bd9247781ef38ad1febe0ba Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Tue, 17 Mar 2020 19:49:41 +0100 Subject: [PATCH 6/9] code review: use references instead of pointers Signed-off-by: Yaroslav Skopets --- source/common/http/async_client_utility.cc | 24 +++++++------------ source/common/http/async_client_utility.h | 4 ++-- test/common/http/async_client_utility_test.cc | 8 +++---- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/source/common/http/async_client_utility.cc b/source/common/http/async_client_utility.cc index dbcab993e23ae..91ef86543d0bf 100644 --- a/source/common/http/async_client_utility.cc +++ b/source/common/http/async_client_utility.cc @@ -9,23 +9,17 @@ AsyncClientRequestTracker::~AsyncClientRequestTracker() { } } -void AsyncClientRequestTracker::add(AsyncClient::Request* request) { - // Let client code to avoid conditionals. - if (request) { - ASSERT(active_requests_.find(request) == active_requests_.end()); - active_requests_.insert(request); - } +void AsyncClientRequestTracker::add(AsyncClient::Request& request) { + ASSERT(active_requests_.find(&request) == active_requests_.end()); + active_requests_.insert(&request); } -void AsyncClientRequestTracker::remove(const AsyncClient::Request* request) { - // Let client code to avoid conditionals. - if (request) { - auto it = active_requests_.find(const_cast(request)); - // Support a use case where request callbacks might get called prior to a request handle - // is returned from AsyncClient::send(). - if (it != active_requests_.end()) { - active_requests_.erase(it); - } +void AsyncClientRequestTracker::remove(const AsyncClient::Request& request) { + auto it = active_requests_.find(const_cast(&request)); + // Support a use case where request callbacks might get called prior to a request handle + // is returned from AsyncClient::send(). + if (it != active_requests_.end()) { + active_requests_.erase(it); } } diff --git a/source/common/http/async_client_utility.h b/source/common/http/async_client_utility.h index a4c836bf0a345..25da625bd061b 100644 --- a/source/common/http/async_client_utility.h +++ b/source/common/http/async_client_utility.h @@ -17,11 +17,11 @@ class AsyncClientRequestTracker { /** * Includes a given async HTTP request into a set of known active requests. */ - void add(AsyncClient::Request* request); + void add(AsyncClient::Request& request); /** * Excludes a given async HTTP request from a set of known active requests. */ - void remove(const AsyncClient::Request* request); + void remove(const AsyncClient::Request& request); private: // Track active async HTTP requests to be able to cancel them on destruction. diff --git a/test/common/http/async_client_utility_test.cc b/test/common/http/async_client_utility_test.cc index ae78f3a074b28..9bc0dead9c852 100644 --- a/test/common/http/async_client_utility_test.cc +++ b/test/common/http/async_client_utility_test.cc @@ -30,11 +30,11 @@ TEST_F(AsyncClientRequestTrackerTest, OnDestructDoNothingIfThereAreNoActiveReque TEST_F(AsyncClientRequestTrackerTest, OnDestructCancelActiveRequests) { // Include active requests. - active_requests_->add(&request1_); - active_requests_->add(&request2_); - active_requests_->add(&request3_); + active_requests_->add(request1_); + active_requests_->add(request2_); + active_requests_->add(request3_); // Exclude active requests. - active_requests_->remove(&request2_); + active_requests_->remove(request2_); // Must cancel active requests on destruction. EXPECT_CALL(request1_, cancel()); From 6f50a54cb807c717bfe1520519dd4ab95d2ea9b3 Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Tue, 17 Mar 2020 22:14:07 +0100 Subject: [PATCH 7/9] Kick CI Signed-off-by: Yaroslav Skopets --- source/common/http/async_client_utility.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/common/http/async_client_utility.h b/source/common/http/async_client_utility.h index 25da625bd061b..27127af4e6cea 100644 --- a/source/common/http/async_client_utility.h +++ b/source/common/http/async_client_utility.h @@ -16,10 +16,12 @@ class AsyncClientRequestTracker { ~AsyncClientRequestTracker(); /** * Includes a given async HTTP request into a set of known active requests. + * @param request request handle */ void add(AsyncClient::Request& request); /** * Excludes a given async HTTP request from a set of known active requests. + * @param request request handle */ void remove(const AsyncClient::Request& request); From cdc881fed9fb5b3960062cbc12e3dff6be025be7 Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Wed, 18 Mar 2020 01:26:30 +0100 Subject: [PATCH 8/9] code review: add comments Signed-off-by: Yaroslav Skopets --- source/common/http/async_client_utility.cc | 3 +++ source/common/http/async_client_utility.h | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/source/common/http/async_client_utility.cc b/source/common/http/async_client_utility.cc index 91ef86543d0bf..3c0807e2e4421 100644 --- a/source/common/http/async_client_utility.cc +++ b/source/common/http/async_client_utility.cc @@ -15,6 +15,9 @@ void AsyncClientRequestTracker::add(AsyncClient::Request& request) { } void AsyncClientRequestTracker::remove(const AsyncClient::Request& request) { + // Notice that use of "const_cast" here is motivated by keeping API convenient for client code. + // In the context where remove() will be typically called, request.cancel() is no longer + // desirable and therefore get prevented by means of "const" modifier. auto it = active_requests_.find(const_cast(&request)); // Support a use case where request callbacks might get called prior to a request handle // is returned from AsyncClient::send(). diff --git a/source/common/http/async_client_utility.h b/source/common/http/async_client_utility.h index 27127af4e6cea..41a98873904c1 100644 --- a/source/common/http/async_client_utility.h +++ b/source/common/http/async_client_utility.h @@ -21,6 +21,15 @@ class AsyncClientRequestTracker { void add(AsyncClient::Request& request); /** * Excludes a given async HTTP request from a set of known active requests. + * + * NOTE: Asymmetry between signatures of add() and remove() is caused by the difference + * between contexts in which these methods will be used. + * add() will be called right after AsyncClient::send() when request.cancel() is + * perfectly valid and desirable. + * However, remove() will be called in the context of + * AsyncClient::Callbacks::[onSuccess | onFailure] where request.cancel() is no longer + * expected and therefore get prevented by means of "const" modifier. + * * @param request request handle */ void remove(const AsyncClient::Request& request); From 31d55a46b0d02919279314e6398021b137e5086d Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Wed, 18 Mar 2020 02:08:25 +0100 Subject: [PATCH 9/9] unit tests: add missing unit test Signed-off-by: Yaroslav Skopets --- source/common/http/async_client_utility.cc | 2 +- test/common/http/async_client_utility_test.cc | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/source/common/http/async_client_utility.cc b/source/common/http/async_client_utility.cc index 3c0807e2e4421..17124f06fb348 100644 --- a/source/common/http/async_client_utility.cc +++ b/source/common/http/async_client_utility.cc @@ -10,7 +10,7 @@ AsyncClientRequestTracker::~AsyncClientRequestTracker() { } void AsyncClientRequestTracker::add(AsyncClient::Request& request) { - ASSERT(active_requests_.find(&request) == active_requests_.end()); + ASSERT(active_requests_.find(&request) == active_requests_.end(), "request is already tracked."); active_requests_.insert(&request); } diff --git a/test/common/http/async_client_utility_test.cc b/test/common/http/async_client_utility_test.cc index 9bc0dead9c852..afcfc0ef0d0e2 100644 --- a/test/common/http/async_client_utility_test.cc +++ b/test/common/http/async_client_utility_test.cc @@ -23,6 +23,11 @@ class AsyncClientRequestTrackerTest : public testing::Test { StrictMock request3_{&async_client_}; }; +TEST_F(AsyncClientRequestTrackerTest, ShouldSupportRemoveWithoutAdd) { + // Should not fail. + active_requests_->remove(request1_); +} + TEST_F(AsyncClientRequestTrackerTest, OnDestructDoNothingIfThereAreNoActiveRequests) { // Trigger destruction. active_requests_.reset();