Skip to content

Http10 without keep-alive means close#74

Merged
jplevyak merged 6 commits intoistio:release-1.1from
jplevyak:http10-means-close
May 31, 2019
Merged

Http10 without keep-alive means close#74
jplevyak merged 6 commits intoistio:release-1.1from
jplevyak:http10-means-close

Conversation

@jplevyak
Copy link

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

jplevyak added 2 commits May 29, 2019 17:14
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
@jplevyak jplevyak changed the title Http10 means close Http10 without keep-alive means close May 30, 2019
@jplevyak jplevyak requested review from PiotrSikora and lambdai May 30, 2019 16:15
Copy link

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

} else if (client.stream_wrapper_->saw_close_header_ || client.codec_client_->remoteClosed()) {
} else if (client.stream_wrapper_->saw_close_header_ || client.codec_client_->remoteClosed() ||
(client.codec_client_->protocol() == Protocol::Http10 &&
!client.stream_wrapper_->saw_keep_alive_header_)) {
Copy link

Choose a reason for hiding this comment

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

Thanks for the update

Copy link
Author

Choose a reason for hiding this comment

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

Added test. Note: this change broke a number of other tests because the test framework was using HTTP/1.0 and not setting keep-alive.

Those are now fixed as well.

PTAL

jplevyak added 2 commits May 30, 2019 12:26
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
@jplevyak jplevyak requested a review from duderino May 30, 2019 20:10
Copy link

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

LGTM, small nit.

saw_close_header_ = true;
parent_.parent_.host_->cluster().stats().upstream_cx_close_notify_.inc();
}
if (absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(),

Choose a reason for hiding this comment

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

This could be else if.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: John Plevyak <jplevyak@gmail.com>
@jplevyak jplevyak merged commit 299c1d1 into istio:release-1.1 May 31, 2019
brian-avery pushed a commit that referenced this pull request Jun 30, 2020
Signed-off-by: Yan Avlasov <yavlasov@google.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.

3 participants