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.

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 another variant of the early close bug #4418

Merged
merged 4 commits into from
Sep 14, 2018

Conversation

zuercher
Copy link
Member

In this case, the early close fix only worked if the upstream connection
hadn't yet been formed. Instead, handle early close as long as there
isn't a pending oneway request. Also discovered a case where the
Unframed transport causes an RPC to start on a zero-length buffer with
end_stream set.

Risk Level: low
Testing: added tests, ran 2000x each
Doc Changes: n/a
Release Notes: n/a
Fixes: #4416

Signed-off-by: Stephan Zuercher [email protected]

In this case, the early close fix only worked if the upstream connection
hadn't yet been formed. Instead, handle early close as long as there
isn't a pending oneway request.

Risk Level: low
Testing: added tests, ran 2000x each
Doc Changes: n/a
Release Notes: n/a
Fixes: envoyproxy#4416

Signed-off-by: Stephan Zuercher <[email protected]>
@zuercher
Copy link
Member Author

@brirams @rgs1 @fishcakez PTAL

@zuercher
Copy link
Member Author

Build failures are circleci-related. Will kick CI once they've recovered.

brirams
brirams previously approved these changes Sep 14, 2018
Copy link
Contributor

@brirams brirams left a comment

Choose a reason for hiding this comment

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

looks reasonable to me

Signed-off-by: Stephan Zuercher <[email protected]>
@zuercher
Copy link
Member Author

@junr03 can take a look? I'd like to get this merged since it fixes a test that's been failing regularly the last couple of days?

TEST_P(ThriftConnManagerIntegrationTest, EarlyCloseWithUpstream) {
initializeCall(DriverMode::Success);

std::string partial_request = request_bytes_.toString().substr(0, request_bytes_.length() - 5);
Copy link
Member

Choose a reason for hiding this comment

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

nit: const

TEST_P(ThriftConnManagerIntegrationTest, OnewayEarlyClosePartialRequest) {
initializeOneway();

std::string partial_request = request_bytes_.toString().substr(0, request_bytes_.length() - 1);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

a few nits, lgtm otherwise.

Signed-off-by: Stephan Zuercher <[email protected]>
@zuercher zuercher merged commit e29d239 into envoyproxy:master Sep 14, 2018
@zuercher zuercher deleted the stephan/fix-earlyclose-test branch September 14, 2018 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

thrift_proxy: integration test is flakey
5 participants