-
Notifications
You must be signed in to change notification settings - Fork 5.5k
buffer: add "overflow" watermark #7619
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.
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
Changes from 1 commit
a87a40e
478b871
c929f30
ef8367c
b5dbfbc
7a96abf
7fe7911
a392911
921e3f0
3ea380a
33e887d
839bfc7
eb3520b
ef8df87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,8 +16,10 @@ namespace Buffer { | |
| class WatermarkBuffer : public OwnedImpl { | ||
| public: | ||
| WatermarkBuffer(std::function<void()> below_low_watermark, | ||
| std::function<void()> above_high_watermark) | ||
| : below_low_watermark_(below_low_watermark), above_high_watermark_(above_high_watermark) {} | ||
| std::function<void()> above_high_watermark, | ||
| std::function<void()> above_overflow_watermark) | ||
| : below_low_watermark_(below_low_watermark), above_high_watermark_(above_high_watermark), | ||
| above_overflow_watermark_(above_overflow_watermark) {} | ||
|
|
||
| // Override all functions from Instance which can result in changing the size | ||
| // of the underlying buffer. | ||
|
|
@@ -35,25 +37,29 @@ class WatermarkBuffer : public OwnedImpl { | |
| Api::IoCallUint64Result write(Network::IoHandle& io_handle) override; | ||
| void postProcess() override { checkLowWatermark(); } | ||
|
|
||
| void setWatermarks(uint32_t watermark) { setWatermarks(watermark / 2, watermark); } | ||
| void setWatermarks(uint32_t low_watermark, uint32_t high_watermark); | ||
| void setWatermarks(uint32_t watermark) { setWatermarks(watermark / 2, watermark, watermark * 2); } | ||
|
mergeconflict marked this conversation as resolved.
Outdated
|
||
| void setWatermarks(uint32_t low_watermark, uint32_t high_watermark, uint32_t overflow_watermark); | ||
| uint32_t highWatermark() const { return high_watermark_; } | ||
|
|
||
| private: | ||
| void checkHighWatermark(); | ||
| void checkHighAndOverflowWatermarks(); | ||
| void checkLowWatermark(); | ||
|
|
||
| std::function<void()> below_low_watermark_; | ||
| std::function<void()> above_high_watermark_; | ||
| std::function<void()> above_overflow_watermark_; | ||
|
|
||
| // Used for enforcing buffer limits (off by default). If these are set to non-zero by a call to | ||
| // setWatermarks() the watermark callbacks will be called as described above. | ||
| uint32_t overflow_watermark_{0}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there no longer a way to set this on a per-buffer basis? Again my concern is that if you have many streams outputting to one downstream H2 connection, and the network::Connection goes over-watermark, that the streams can each dump roughly one watermark worth of data into the network::connection without being malicious. I think when we talk about memory limits per stream this is accounted for, and we have to make sure that the H2 HCM can set this for downstream H2 and the connection pool can set a higher multiplier for H2 upstream. |
||
| uint32_t high_watermark_{0}; | ||
| uint32_t low_watermark_{0}; | ||
| // Tracks the latest state of watermark callbacks. | ||
| // True between the time above_high_watermark_ has been called until above_high_watermark_ has | ||
| // been called. | ||
| // Set to true after above_high_watermark_ has been called, and reset to false after | ||
| // below_low_watermark_ has been called. | ||
| bool above_high_watermark_called_{false}; | ||
| // Set to true after above_overflow_watermark_ has been called. Never reset, because we assume | ||
| // the stream will be forcibly closed in response. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. object will be destroyed? I don't think we want this stream-centric given it could be a connection
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, which object will be destroyed?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My point is that in a buffer which is used by http streams, raw tcp connections, and arbitrary other objects, we shouldn't be talking about streams :-) |
||
| bool above_overflow_watermark_called_{false}; | ||
| }; | ||
|
|
||
| using WatermarkBufferPtr = std::unique_ptr<WatermarkBuffer>; | ||
|
|
@@ -62,8 +68,10 @@ class WatermarkBufferFactory : public WatermarkFactory { | |
| public: | ||
| // Buffer::WatermarkFactory | ||
| InstancePtr create(std::function<void()> below_low_watermark, | ||
| std::function<void()> above_high_watermark) override { | ||
| return InstancePtr{new WatermarkBuffer(below_low_watermark, above_high_watermark)}; | ||
| std::function<void()> above_high_watermark, | ||
| std::function<void()> above_overflow_watermark) override { | ||
| return InstancePtr{ | ||
| new WatermarkBuffer(below_low_watermark, above_high_watermark, above_overflow_watermark)}; | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1649,6 +1649,11 @@ void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason, absl: | |
| connection_manager_.doDeferredStreamDestroy(*this); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::onAboveWriteBufferOverflowWatermark() { | ||
| ENVOY_STREAM_LOG(debug, "Closing upstream stream due to downstream stream overflow.", *this); | ||
| callOverflowWatermarkCallbacks(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::onAboveWriteBufferHighWatermark() { | ||
| ENVOY_STREAM_LOG(debug, "Disabling upstream stream due to downstream stream watermark.", *this); | ||
| callHighWatermarkCallbacks(); | ||
|
|
@@ -1672,6 +1677,12 @@ bool ConnectionManagerImpl::ActiveStream::verbose() const { | |
| return connection_manager_.config_.tracingConfig()->verbose_; | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::callOverflowWatermarkCallbacks() { | ||
| for (auto watermark_callbacks : watermark_callbacks_) { | ||
| watermark_callbacks->onAboveWriteBufferOverflowWatermark(); | ||
| } | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::callHighWatermarkCallbacks() { | ||
| ++high_watermark_count_; | ||
| for (auto watermark_callbacks : watermark_callbacks_) { | ||
|
|
@@ -1939,9 +1950,9 @@ void ConnectionManagerImpl::ActiveStreamFilterBase::clearRouteCache() { | |
| } | ||
|
|
||
| Buffer::WatermarkBufferPtr ConnectionManagerImpl::ActiveStreamDecoderFilter::createBuffer() { | ||
| auto buffer = | ||
| std::make_unique<Buffer::WatermarkBuffer>([this]() -> void { this->requestDataDrained(); }, | ||
| [this]() -> void { this->requestDataTooLarge(); }); | ||
| auto buffer = std::make_unique<Buffer::WatermarkBuffer>( | ||
| [this]() -> void { this->requestDataDrained(); }, | ||
| [this]() -> void { this->requestDataTooLarge(); }, [this]() -> void { this->resetStream(); }); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will need to refresh my memory on this code a bit, but some thought will need to be put into how we reset the stream here. Should it look like a remote reset? Local reset? What reset code do we use? Etc. Mainly just a heads up to think about this a bit. Same below. (You might consider moving buffer creation to a shared function with more comments.) Will also need a stat here. |
||
| buffer->setWatermarks(parent_.buffer_limit_); | ||
| return buffer; | ||
| } | ||
|
|
@@ -2013,6 +2024,13 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::encodeMetadata( | |
| parent_.encodeMetadata(nullptr, std::move(metadata_map_ptr)); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter:: | ||
| onDecoderFilterAboveWriteBufferOverflowWatermark() { | ||
| ENVOY_STREAM_LOG(debug, "Closing downstream stream due to filter callbacks.", parent_); | ||
| // TODO(mergeconflict): Add a new flow control stat. | ||
| resetStream(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter:: | ||
| onDecoderFilterAboveWriteBufferHighWatermark() { | ||
| ENVOY_STREAM_LOG(debug, "Read-disabling downstream stream due to filter callbacks.", parent_); | ||
|
|
@@ -2086,7 +2104,8 @@ bool ConnectionManagerImpl::ActiveStreamDecoderFilter::recreateStream() { | |
|
|
||
| Buffer::WatermarkBufferPtr ConnectionManagerImpl::ActiveStreamEncoderFilter::createBuffer() { | ||
| auto buffer = new Buffer::WatermarkBuffer([this]() -> void { this->responseDataDrained(); }, | ||
| [this]() -> void { this->responseDataTooLarge(); }); | ||
| [this]() -> void { this->responseDataTooLarge(); }, | ||
| [this]() -> void { this->resetStream(); }); | ||
| buffer->setWatermarks(parent_.buffer_limit_); | ||
| return Buffer::WatermarkBufferPtr{buffer}; | ||
| } | ||
|
|
@@ -2106,6 +2125,12 @@ HeaderMap& ConnectionManagerImpl::ActiveStreamEncoderFilter::addEncodedTrailers( | |
| return parent_.addEncodedTrailers(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamEncoderFilter:: | ||
| onEncoderFilterAboveWriteBufferOverflowWatermark() { | ||
| ENVOY_STREAM_LOG(debug, "Closing upstream stream due to filter callbacks.", parent_); | ||
| parent_.callOverflowWatermarkCallbacks(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamEncoderFilter:: | ||
| onEncoderFilterAboveWriteBufferHighWatermark() { | ||
| ENVOY_STREAM_LOG(debug, "Disabling upstream stream due to filter callbacks.", parent_); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.