Skip to content
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

Jump to bottom

upstream: avoid reset after end_stream #14106

Merged
merged 8 commits into from
Dec 1, 2020
Merged

upstream: avoid reset after end_stream #14106

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2d3bcfb
upstream: avoid reset after end_stream
snowp Nov 19, 2020
1088604
defer delete the upstream to prevent inline deletion
snowp Nov 23, 2020
a63edbf
fix deferred deletions in test
snowp Nov 24, 2020
6d9f5cb
format
snowp Nov 24, 2020
a00f00b
defensively check upstream_request_ a few more places
snowp Nov 24, 2020
999f575
error status
snowp Nov 30, 2020
cb63c1c
format
snowp Nov 30, 2020
a1a6a0e
fix tests
snowp Dec 1, 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
4 changes: 2 additions & 2 deletions include/envoy/router/router.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "envoy/config/core/v3/base.pb.h"
#include "envoy/config/route/v3/route_components.pb.h"
#include "envoy/config/typed_metadata.h"
#include "envoy/event/deferred_deletable.h"
#include "envoy/http/codec.h"
#include "envoy/http/codes.h"
#include "envoy/http/conn_pool.h"
Expand Down Expand Up @@ -1256,9 +1257,8 @@ class GenericConnectionPoolCallbacks {
*
* It is similar logically to RequestEncoder, only without the getStream interface.
*/
class GenericUpstream {
class GenericUpstream : public Event::DeferredDeletable {
public:
virtual ~GenericUpstream() = default;
/**
* Encode a data frame.
* @param data supplies the data to encode. The data may be moved by the encoder.
Expand Down
2 changes: 1 addition & 1 deletion source/common/router/upstream_request.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ void UpstreamRequest::clearRequestEncoder() {
if (upstream_) {
parent_.callbacks()->removeDownstreamWatermarkCallbacks(downstream_watermark_manager_);
}
upstream_.reset();
parent_.callbacks()->dispatcher().deferredDelete(std::move(upstream_));
}

void UpstreamRequest::DownstreamWatermarkManager::onAboveWriteBufferHighWatermark() {
Expand Down
5 changes: 5 additions & 0 deletions source/extensions/upstreams/http/tcp/upstream_request.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ void TcpUpstream::resetStream() {

void TcpUpstream::onUpstreamData(Buffer::Instance& data, bool end_stream) {
upstream_request_->decodeData(data, end_stream);
// This ensures that if we get a reset after end_stream we won't propagate two
// "end streams" to the upstream_request_.
if (end_stream) {
Copy link
Contributor

Choose a reason for hiding this comment

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

once we have upstream filters, will it be possible to get an upstream "end stream "before we send headers? I think it could be the case, so I think we're going to want to do null pointer checks for upstream_request in few other places, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point, I'll go over all the functions

upstream_request_ = nullptr;
}
}

void TcpUpstream::onEvent(Network::ConnectionEvent event) {
Expand Down
9 changes: 9 additions & 0 deletions test/extensions/upstreams/http/tcp/upstream_request_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,15 @@ TEST_F(TcpUpstreamTest, V2Header) {
tcp_upstream_->encodeData(buffer, false);
}

// Verifies that a reset after end_stream=true doesn't trigger a callback
// on the router filter.
TEST_F(TcpUpstreamTest, ResetAfterEndStream) {
Buffer::OwnedImpl buffer("something");
EXPECT_CALL(mock_router_filter_, onUpstreamData(BufferStringEqual("something"), _, true));
tcp_upstream_->onUpstreamData(buffer, true);
tcp_upstream_->onEvent(Network::ConnectionEvent::RemoteClose);
}

TEST_F(TcpUpstreamTest, TrailersEndStream) {
// Swallow the headers.
EXPECT_TRUE(tcp_upstream_->encodeHeaders(request_, false).ok());
Expand Down