Skip to content

async client: introduce AsyncClientRequestTracker to keep track of inflight requests and cancel them on destruction #10359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 9 commits into from
Mar 18, 2020
Merged

async client: introduce AsyncClientRequestTracker to keep track of inflight requests and cancel them on destruction #10359

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9117dbd
async client: introduce `AsyncClientRequestTracker` to keep track of …
yskopets Mar 12, 2020
943a499
Kick CI
yskopets Mar 15, 2020
3136d4d
code review: use regular methods instead of operator overloading
yskopets Mar 16, 2020
6c986b3
code review: use `absl::flat_hash_set` instead of `std::unordered_set`
yskopets Mar 16, 2020
1d04f5f
code review: change return type to `void`
yskopets Mar 17, 2020
efeab01
code review: use references instead of pointers
yskopets Mar 17, 2020
6f50a54
Kick CI
yskopets Mar 17, 2020
cdc881f
code review: add comments
yskopets Mar 18, 2020
31d55a4
unit tests: add missing unit test
yskopets Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions source/common/http/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
27 changes: 27 additions & 0 deletions source/common/http/async_client_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#include "common/http/async_client_utility.h"

namespace Envoy {
namespace Http {

AsyncClientRequestTracker::~AsyncClientRequestTracker() {
for (auto* active_request : active_requests_) {
active_request->cancel();
}
}

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) {
auto it = active_requests_.find(const_cast<AsyncClient::Request*>(&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);
}
}

} // namespace Http
} // namespace Envoy
34 changes: 34 additions & 0 deletions source/common/http/async_client_utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#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.
* @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);

@mattklein123 mattklein123 Mar 17, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to pass a non-const object above and a const object here. I would just pass a non-const in both places and remove the const_cast. Thank you!

/wait

@yskopets yskopets Mar 17, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AsyncClientRequestTracker::remove() is intended for use in the context of AsyncClient::Callbacks::onSuccess(request, response) and AsyncClient::Callbacks::onFailure(request, reason).

Notice that onSuccess / onFailure methods will now have request as an extra parameter to enable correlation between requests and responses.

The proposed interface for AsyncClient::Callbacks looks like this:

  /**
   * Notifies caller of async HTTP request status.
   *
   * To support a use case where a caller makes multiple requests in parallel,
   * individual callback methods provide request context corresponding to that response.
   */
  class Callbacks {
  public:
    virtual ~Callbacks() = default;

    /**
     * Called when the async HTTP request succeeds.
     * @param request  request handle.
     *                 NOTE: request handle is passed for correlation purposes only, e.g.
     *                 for client code to be able to exclude that handle from a list of
     *                 requests in progress.
     * @param response the HTTP response
     */
    virtual void onSuccess(const Request& request, ResponseMessagePtr&& response) PURE;

    /**
     * Called when the async HTTP request fails.
     * @param request request handle.
     *                NOTE: request handle is passed for correlation purposes only, e.g.
     *                for client code to be able to exclude that handle from a list of
     *                requests in progress.
     * @param reason  failure reason
     */
    virtual void onFailure(const Request& request, FailureReason reason) PURE;
  };

request was made const Request& to prevent any unintended usage, e.g. calling request.cancel().

So, const Request& in Callbacks causes the same in AsyncClientRequestTracker::remove() (otherwise, const_cast will be a part of every call to AsyncClientRequestTracker::remove()).

Do you want me to remove const AsyncClient::Request& from both AsyncClientRequestTracker and Callbacks ?

@mattklein123 mattklein123 Mar 17, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm OK. Can you add a comment above the const_cast as to why it's required? Thank you.

@yskopets yskopets Mar 18, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comments


private:
// Track active async HTTP requests to be able to cancel them on destruction.
absl::flat_hash_set<AsyncClient::Request*> active_requests_;
};

} // namespace Http
} // namespace Envoy
9 changes: 9 additions & 0 deletions test/common/http/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
49 changes: 49 additions & 0 deletions test/common/http/async_client_utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<AsyncClientRequestTracker> active_requests_{
std::make_unique<AsyncClientRequestTracker>()};

NiceMock<MockAsyncClient> async_client_;
StrictMock<MockAsyncClientRequest> request1_{&async_client_};
StrictMock<MockAsyncClientRequest> request2_{&async_client_};
StrictMock<MockAsyncClientRequest> request3_{&async_client_};
};

TEST_F(AsyncClientRequestTrackerTest, OnDestructDoNothingIfThereAreNoActiveRequests) {
// Trigger destruction.
active_requests_.reset();
}

TEST_F(AsyncClientRequestTrackerTest, OnDestructCancelActiveRequests) {
// Include active requests.
active_requests_->add(request1_);
active_requests_->add(request2_);
active_requests_->add(request3_);
// Exclude active requests.
active_requests_->remove(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