Skip to content

http2: transist READY client to busy when SETTING decreases the capacity #18999

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
ggreenway merged 3 commits into from
Nov 17, 2021
Merged

http2: transist READY client to busy when SETTING decreases the capacity #18999

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0838dac
http2: transist READY client to busy when SETTING decreases the capacity
YaoZengzeng Nov 13, 2021
53145bd
add some comments
YaoZengzeng Nov 13, 2021
ea30da9
fix comment and test code
YaoZengzeng Nov 16, 2021
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
2 changes: 1 addition & 1 deletion source/common/conn_pool/conn_pool_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ void ConnPoolImplBase::onStreamClosed(Envoy::ConnectionPool::ActiveClient& clien
if (client.state() == ActiveClient::State::DRAINING && client.numActiveStreams() == 0) {
// Close out the draining client if we no longer have active streams.
client.close();
} else if (client.state() == ActiveClient::State::BUSY && client.currentUnusedCapacity() != 0) {
} else if (client.state() == ActiveClient::State::BUSY && client.currentUnusedCapacity() > 0) {
transitionActiveClientState(client, ActiveClient::State::READY);
if (!delay_attaching_stream) {
onUpstreamReady();
Expand Down
3 changes: 3 additions & 0 deletions source/common/http/conn_pool_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ void MultiplexedActiveClientBase::onSettings(ReceivedSettings& settings) {
ASSERT(std::numeric_limits<int32_t>::max() >= old_unused_capacity);
concurrent_stream_limit_ = settings.maxConcurrentStreams().value();
int64_t delta = old_unused_capacity - currentUnusedCapacity();
if (state() == ActiveClient::State::READY && currentUnusedCapacity() <= 0) {
parent_.transitionActiveClientState(*this, ActiveClient::State::BUSY);
}
parent_.decrClusterStreamCapacity(delta);
ENVOY_CONN_LOG(trace, "Decreasing stream capacity by {}", *codec_client_, delta);
negative_capacity_ += delta;
Expand Down
30 changes: 23 additions & 7 deletions test/common/http/http2/conn_pool_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,30 +1436,46 @@ TEST_F(Http2ConnPoolImplTest, PreconnectWithoutMultiplexing) {

TEST_F(Http2ConnPoolImplTest, DisconnectWithNegativeCapacity) {
TestScopedRuntime scoped_runtime;
cluster_->http2_options_.mutable_max_concurrent_streams()->set_value(2);
cluster_->http2_options_.mutable_max_concurrent_streams()->set_value(4);
ON_CALL(*cluster_, perUpstreamPreconnectRatio).WillByDefault(Return(1));

// One stream results in one connection. Two streams result in two connections.

@alyssawilk alyssawilk Nov 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also not yours, but I think this comment is from another test and should be removed here (as long as I'm looking at it)b

expectClientsCreate(1);
ActiveTestRequest r1(*this, 0, false);
CHECK_STATE(0 /*active*/, 1 /*pending*/, 2 /*capacity*/);
CHECK_STATE(0 /*active*/, 1 /*pending*/, 4 /*capacity*/);
ActiveTestRequest r2(*this, 0, false);
CHECK_STATE(0 /*active*/, 2 /*pending*/, 2 /*capacity*/);
CHECK_STATE(0 /*active*/, 2 /*pending*/, 4 /*capacity*/);
ActiveTestRequest r3(*this, 0, false);
CHECK_STATE(0 /*active*/, 3 /*pending*/, 4 /*capacity*/);

// When the connection connects, there is zero spare capacity in this pool.

@alyssawilk alyssawilk Nov 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you need to update comments?
Before we had max concurrent == 2 and 2 streams, so 0 capacity.
now we have max concurrent == 3 and 3 streams, so 1 capacity left?

EXPECT_CALL(*test_clients_[0].codec_, newStream(_))
.WillOnce(DoAll(SaveArgAddress(&r1.inner_decoder_), ReturnRef(r1.inner_encoder_)))
.WillOnce(DoAll(SaveArgAddress(&r2.inner_decoder_), ReturnRef(r2.inner_encoder_)));
.WillOnce(DoAll(SaveArgAddress(&r2.inner_decoder_), ReturnRef(r2.inner_encoder_)))
.WillOnce(DoAll(SaveArgAddress(&r3.inner_decoder_), ReturnRef(r3.inner_encoder_)));
EXPECT_CALL(r1.callbacks_.pool_ready_, ready());
EXPECT_CALL(r2.callbacks_.pool_ready_, ready());
EXPECT_CALL(r3.callbacks_.pool_ready_, ready());
expectClientConnect(0);
CHECK_STATE(2 /*active*/, 0 /*pending*/, 0 /*capacity*/);
CHECK_STATE(3 /*active*/, 0 /*pending*/, 1 /*capacity*/);
EXPECT_EQ(pool_->owningList(Envoy::ConnectionPool::ActiveClient::State::READY).size(), 1);

// Settings frame reducing capacity to one stream per connection results in -1 capacity.
// Settings frame reducing capacity to one stream per connection results in -2 capacity.
NiceMock<MockReceivedSettings> settings;
settings.max_concurrent_streams_ = 1;
test_clients_[0].codec_client_->onSettings(settings);
CHECK_STATE(2 /*active*/, 0 /*pending*/, -1 /*capacity*/);
CHECK_STATE(3 /*active*/, 0 /*pending*/, -2 /*capacity*/);
EXPECT_EQ(pool_->owningList(Envoy::ConnectionPool::ActiveClient::State::READY).size(), 0);

// If one stream closes, concurrency capacity goes to -1, still no ready client available.
completeRequest(r1);
EXPECT_EQ(pool_->owningList(Envoy::ConnectionPool::ActiveClient::State::READY).size(), 0);

// Close all streams, concurrency capacity goes to -1, there should be one ready client.

@alyssawilk alyssawilk Nov 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

concurrency capacity goes to 1, not -1 right?

completeRequest(r2);
completeRequest(r3);
EXPECT_EQ(pool_->owningList(Envoy::ConnectionPool::ActiveClient::State::READY).size(), 1);
CHECK_STATE(0 /*active*/, 0 /*pending*/, 1 /*capacity*/);

@alyssawilk alyssawilk Nov 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you mind creating more streams before doing the close? I think there's value in regression testing that close works when capacity is negative.


closeAllClients();
}
Expand Down