-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
thrift_proxy: fix oneway bugs #4025
thrift_proxy: fix oneway bugs #4025
Conversation
The thrift_proxy had some bugs related to oneway messages, both when there is an upstream connection pool failure and when the downstream connection closes before the pool connection completes. Both are fixed with theses changes. In addition, modifies the test harness server to accept multiple simulataneous connections. *Risk Level*: low *Testing*: integration test added *Docs Changes*: n/a *Release Notes*: n/a Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
N.B. this will conflict with #3991 |
@@ -281,7 +286,6 @@ bool ConnectionManager::ActiveRpc::upstreamData(Buffer::Instance& buffer) { | |||
|
|||
void ConnectionManager::ActiveRpc::resetDownstreamConnection() { | |||
parent_.read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); | |||
parent_.doDeferredRpcDestroy(*this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this line removed, where does the cleanup happen instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the downstream connection is closed, ConnectionManager::onEvent is invoked, which will destroy any items in the rpcs_ list. What was happening before was that the RPC was being destroyed but not removed from rpcs_.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Which causes a subsequent nullptr dereference.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, now it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes!
@@ -281,7 +286,6 @@ bool ConnectionManager::ActiveRpc::upstreamData(Buffer::Instance& buffer) { | |||
|
|||
void ConnectionManager::ActiveRpc::resetDownstreamConnection() { | |||
parent_.read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); | |||
parent_.doDeferredRpcDestroy(*this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, now it makes sense.
(I'm holding off fixing the merge error until #4067 lands, since its reversion is the source of the conflict.) |
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
* origin/master: (38 commits) test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114) perf: reduce the memory usage of LC Trie construction (envoyproxy#4117) test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127) test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141) rbac: add rbac network filter. (envoyproxy#4083) fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116) Set content-type and content-length (envoyproxy#4113) fault: use FractionalPercent for percent (envoyproxy#3978) test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134) Added cluster_name to load assignment config for static cluster (envoyproxy#4123) ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115) syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111) thrift_proxy: fix oneway bugs (envoyproxy#4025) Do not crash when converting YAML to JSON fails (envoyproxy#4110) config: allow unknown fields flag (take 2) (envoyproxy#4096) Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108) bazel: use GCS remote cache (envoyproxy#4050) Add thread local cache of overload action states (envoyproxy#4090) Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079) secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>
The thrift_proxy had some bugs related to oneway messages, both when
there is an upstream connection pool failure and when the downstream
connection closes before the pool connection completes. Both are fixed
with theses changes. In addition, modifies the test harness server to
accept multiple simulataneous connections.
Risk Level: low
Testing: integration test added
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Stephan Zuercher stephan@turbinelabs.io