Skip to content

Fix connection reuse by delaying a poll cycle. #73

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
jplevyak merged 5 commits into from
May 31, 2019
Merged

Fix connection reuse by delaying a poll cycle. #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0dedd11
Fix connection reuse by delaying a poll cycle.
jplevyak May 29, 2019
ccbf3a5
Delay 2 cycles and reenable independent of pending requests.
jplevyak May 29, 2019
27f1810
Address comments.
jplevyak May 30, 2019
ec73477
Address formatting.
jplevyak May 30, 2019
c6ac813
Address comments.
jplevyak May 31, 2019
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
35 changes: 32 additions & 3 deletions source/common/http/http1/conn_pool.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ ConnPoolImpl::ConnPoolImpl(Event::Dispatcher& dispatcher, Upstream::HostConstSha
upstream_ready_timer_(dispatcher_.createTimer([this]() { onUpstreamReady(); })) {}

ConnPoolImpl::~ConnPoolImpl() {
while (!delayed_clients_.empty()) {
delayed_clients_.front()->codec_client_->close();
}

while (!ready_clients_.empty()) {
ready_clients_.front()->codec_client_->close();
}
Expand Down Expand Up @@ -147,7 +151,12 @@ void ConnPoolImpl::onConnectionEvent(ActiveClient& client, Network::ConnectionEv
} else if (!client.connect_timer_) {
// The connect timer is destroyed on connect. The lack of a connect timer means that this
// client is idle and in the ready pool.
removed = client.removeFromList(ready_clients_);
if (client.delayed_) {
client.delayed_ = 0;
removed = client.removeFromList(delayed_clients_);
} else {
removed = client.removeFromList(ready_clients_);
}
check_for_drained = false;
} else {
// The only time this happens is if we actually saw a connect failure.
Expand Down Expand Up @@ -220,6 +229,20 @@ void ConnPoolImpl::onResponseComplete(ActiveClient& client) {

void ConnPoolImpl::onUpstreamReady() {
upstream_ready_enabled_ = false;
auto it = delayed_clients_.begin();
while (it != delayed_clients_.end()) {
ActiveClient& client = **it;
it++; // Move forward before moveBetweenLists which would invalidate 'it'.
client.delayed_--;
if (client.delayed_ == 0) {
ENVOY_CONN_LOG(debug, "moving from delay to ready", *client.codec_client_);
client.moveBetweenLists(delayed_clients_, ready_clients_);
}
Copy link

@lambdai lambdai May 30, 2019

Choose a reason for hiding this comment

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

could you create another time here when

  1. the delayed_clients is not empty, and
  2. there is no pending timer
    So that the delayed clients can be moved to ready in next batch.

Copy link
Author

@jplevyak jplevyak May 31, 2019

Choose a reason for hiding this comment

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

Done.

}
if (!delayed_clients_.empty()) {
upstream_ready_enabled_ = true;
upstream_ready_timer_->enableTimer(std::chrono::milliseconds(0));
}
while (!pending_requests_.empty() && !ready_clients_.empty()) {
ActiveClient& client = *ready_clients_.front();
ENVOY_CONN_LOG(debug, "attaching to next request", *client.codec_client_);
Expand All @@ -234,7 +257,13 @@ void ConnPoolImpl::onUpstreamReady() {

void ConnPoolImpl::processIdleClient(ActiveClient& client, bool delay) {
client.stream_wrapper_.reset();
if (pending_requests_.empty() || delay) {
if (delay) {
ENVOY_CONN_LOG(debug, "moving to delay", *client.codec_client_);
// N.B. libevent does not guarantee ordering of events, so to ensure that the delayed client
// experiences a poll cycle before being made ready, delay for 2 event loops.
client.delayed_ = 2;
client.moveBetweenLists(busy_clients_, delayed_clients_);
} else if (pending_requests_.empty()) {
// There is nothing to service or delayed processing is requested, so just move the connection
// into the ready list.
ENVOY_CONN_LOG(debug, "moving to ready", *client.codec_client_);
Expand All @@ -248,7 +277,7 @@ void ConnPoolImpl::processIdleClient(ActiveClient& client, bool delay) {
pending_requests_.pop_back();
}

if (delay && !pending_requests_.empty() && !upstream_ready_enabled_) {
if (!delayed_clients_.empty() && !upstream_ready_enabled_) {
upstream_ready_enabled_ = true;
upstream_ready_timer_->enableTimer(std::chrono::milliseconds(0));
}
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/http1/conn_pool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ class ConnPoolImpl : public ConnectionPool::Instance, public ConnPoolImplBase {
Event::TimerPtr connect_timer_;
Stats::TimespanPtr conn_length_;
uint64_t remaining_requests_;
int delayed_{0};
};

typedef std::unique_ptr<ActiveClient> ActiveClientPtr;
Expand All @@ -117,6 +118,7 @@ class ConnPoolImpl : public ConnectionPool::Instance, public ConnPoolImplBase {

Stats::TimespanPtr conn_connect_ms_;
Event::Dispatcher& dispatcher_;
std::list<ActiveClientPtr> delayed_clients_;
std::list<ActiveClientPtr> ready_clients_;
std::list<ActiveClientPtr> busy_clients_;
std::list<DrainedCb> drained_callbacks_;
Expand Down