Skip to content

health check: fix additional regression from #6765#6942

Merged
mattklein123 merged 1 commit intomasterfrom
fix_hc
May 15, 2019
Merged

health check: fix additional regression from #6765#6942
mattklein123 merged 1 commit intomasterfrom
fix_hc

Conversation

@mattklein123
Copy link
Member

If we inline delete a host during a failure callback we need to
account for the connection being cleaned up prior to handling
'connection: close' headers.

Risk Level: Low
Testing: New UT and modified integration test
Docs Changes: N/A
Release Notes: N/A

If we inline delete a host during a failure callback we need to
account for the connection being cleaned up prior to handling
'connection: close' headers.

Signed-off-by: Matt Klein <mklein@lyft.com>
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM with one question


request_encoder_ = &client_->newStream(*this);
request_encoder_->getStream().addCallbacks(*this);
Http::StreamEncoder* request_encoder = &client_->newStream(*this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a cleanup or does is this part of the fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a cleanup. The member variable was not used so I deleted it.

@mattklein123 mattklein123 merged commit 6dd4b6f into master May 15, 2019
@mattklein123 mattklein123 deleted the fix_hc branch May 15, 2019 00:01
rgs1 pushed a commit to rgs1/envoy that referenced this pull request May 15, 2019
This is a follow-up to envoyproxy#6942. This makes it a bit more readable. We
spent some time debugging this too, so it would be nice to ensure
it's readable next time we bump into something around this code
surface :-)

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
mattklein123 pushed a commit that referenced this pull request May 15, 2019
This is a follow-up to #6942. This makes it a bit more readable. We
spent some time debugging this too, so it would be nice to ensure
it's readable next time we bump into something around this code
surface :-)

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
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.

2 participants