Skip to content

refactor router filter to store upstream requests in a list. #6540

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 7 commits into from
Apr 16, 2019
Merged

refactor router filter to store upstream requests in a list. #6540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
42149c6
refactor router filter to store upstream requests in a list.
mpuncel Apr 10, 2019
6be4b93
fix heap use after free
mpuncel Apr 10, 2019
8a9e93e
fix variable redeclaration
mpuncel Apr 10, 2019
ed85803
use list to store watermark callbacks
mpuncel Apr 11, 2019
5c66169
enhance existing watermark unit tests to exercise multiple subscriber…
mpuncel Apr 15, 2019
f86b421
fix assert in router
mpuncel Apr 15, 2019
679f4d9
prefer avoiding a raw pointer
mpuncel Apr 16, 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
23 changes: 12 additions & 11 deletions source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1547,16 +1547,16 @@ bool ConnectionManagerImpl::ActiveStream::verbose() const {

void ConnectionManagerImpl::ActiveStream::callHighWatermarkCallbacks() {
++high_watermark_count_;
if (watermark_callbacks_) {
Copy link
Contributor

@alyssawilk alyssawilk Apr 11, 2019

Choose a reason for hiding this comment

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

I think we should enhance existing unit tests to have two subscribers, to regression test both get the callback

watermark_callbacks_->onAboveWriteBufferHighWatermark();
for (auto watermark_callbacks : watermark_callbacks_) {
watermark_callbacks->onAboveWriteBufferHighWatermark();
}
}

void ConnectionManagerImpl::ActiveStream::callLowWatermarkCallbacks() {
ASSERT(high_watermark_count_ > 0);
Copy link
Contributor

@alyssawilk alyssawilk Apr 11, 2019

Choose a reason for hiding this comment

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

I'm looking at this and I think there may be a pre-existing bug which worked OK before because there was one back-up cause, but will not work with two.

If you have the upstream connection call high watermark callbacks, and increment high_watermark_count_, then the hedge connection hits its watermark and increments high_watermark_count_, I don't think we want to resume by calling the low watermark callbacks until the count is back to 0.

If I'm correct here we may have been overenthusiastic resuming, but fixing will be a fairly high risk change.

Copy link
Contributor Author

@mpuncel mpuncel Apr 11, 2019

Choose a reason for hiding this comment

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

is the reason the fix is high risk because the count might not reach 0 if there is a counting bug somewhere?

Copy link
Contributor

@alyssawilk alyssawilk Apr 11, 2019

Choose a reason for hiding this comment

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

Yep. I mean you can land this and do the other separately but I don't think you can land your hedge fixes without both, and the fix is high risk because it may be masking other bugs.

--high_watermark_count_;
if (watermark_callbacks_) {
watermark_callbacks_->onBelowWriteBufferLowWatermark();
for (auto watermark_callbacks : watermark_callbacks_) {
watermark_callbacks->onBelowWriteBufferLowWatermark();
}
}

Expand Down Expand Up @@ -1891,19 +1891,20 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::

void ConnectionManagerImpl::ActiveStreamDecoderFilter::addDownstreamWatermarkCallbacks(
DownstreamWatermarkCallbacks& watermark_callbacks) {
// This is called exactly once per stream, by the router filter.
// If there's ever a need for another filter to subscribe to watermark callbacks this can be
// turned into a vector.
ASSERT(parent_.watermark_callbacks_ == nullptr);
parent_.watermark_callbacks_ = &watermark_callbacks;
// This is called exactly once per upstream-stream, by the router filter. Therefore, we
// expect the same callbacks to not be registered twice.
ASSERT(std::find(parent_.watermark_callbacks_.begin(), parent_.watermark_callbacks_.end(),
&watermark_callbacks) == parent_.watermark_callbacks_.end());
parent_.watermark_callbacks_.emplace(parent_.watermark_callbacks_.end(), &watermark_callbacks);
for (uint32_t i = 0; i < parent_.high_watermark_count_; ++i) {
watermark_callbacks.onAboveWriteBufferHighWatermark();
}
}
void ConnectionManagerImpl::ActiveStreamDecoderFilter::removeDownstreamWatermarkCallbacks(
Copy link
Contributor

@alyssawilk alyssawilk Apr 11, 2019

Choose a reason for hiding this comment

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

can you poke through code and make sure if upstream connection 1 is above the high watermark (and causes the state to transition to high watermark) and upstream connection 2 ends up paused, that if upstream connection 1 goes away that it clears the state so that 2 ends up resuming? We want to make sure we don't get wedged here.

DownstreamWatermarkCallbacks& watermark_callbacks) {
ASSERT(parent_.watermark_callbacks_ == &watermark_callbacks);
parent_.watermark_callbacks_ = nullptr;
ASSERT(std::find(parent_.watermark_callbacks_.begin(), parent_.watermark_callbacks_.end(),
&watermark_callbacks) != parent_.watermark_callbacks_.end());
parent_.watermark_callbacks_.remove(&watermark_callbacks);
}

bool ConnectionManagerImpl::ActiveStreamDecoderFilter::recreateStream() {
Expand Down
2 changes: 1 addition & 1 deletion source/common/http/conn_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
StreamInfo::StreamInfoImpl stream_info_;
absl::optional<Router::RouteConstSharedPtr> cached_route_;
absl::optional<Upstream::ClusterInfoConstSharedPtr> cached_cluster_info_;
DownstreamWatermarkCallbacks* watermark_callbacks_{nullptr};
std::list<DownstreamWatermarkCallbacks*> watermark_callbacks_{};
uint32_t buffer_limit_{0};
uint32_t high_watermark_count_{0};
const std::string* decorated_operation_{nullptr};
Expand Down
1 change: 1 addition & 0 deletions source/common/router/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ envoy_cc_library(
"//source/common/common:enum_to_int",
"//source/common/common:hash_lib",
"//source/common/common:hex_lib",
"//source/common/common:linked_object",
"//source/common/common:minimal_logger_lib",
"//source/common/common:utility_lib",
"//source/common/grpc:common_lib",
Expand Down
Loading