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

Clean-up the HTTP versions check in response parsing, and don't reuse connections on any response other than HTTP/1.1 #5608

Merged
merged 5 commits into from
May 20, 2024

Conversation

ahsonkhan
Copy link
Member

@ahsonkhan ahsonkhan commented May 8, 2024

Update: Other than making the HTTP version checks a bit easier to understand (using single conditional if, as suggested by @LarryOsterman), the primary change here is to not persist a connection if we received an invalid/unsupported HTTP version in the response, i.e. if the HTTP version does not start with 1.x. We could throw here instead, but at least marking it as don't re-use, is a good thing to do.
The logic for 1.x remains unchanged, the semantics are the same, just written in a way that's easier to understand.

These are the particular HTTP versions: 0.9, 1.0, 1.1, 2.0, 3.0.
1.1 is supported by the SDK, 2.0/3.0 are currently not supported. We can make 1.0 work, in this case, regarding connection persistence defaults.

There's no HTTP 1.1+, so hardening the version checks to pick the concrete protocol versions. And rejecting others (including the case which indicates a malformed response).

…ically for 1.1 or 1.0, and throwing otherwise.
sdk/core/azure-core/src/http/curl/curl.cpp Outdated Show resolved Hide resolved
sdk/core/azure-core/src/http/curl/curl.cpp Outdated Show resolved Hide resolved
sdk/core/azure-core/src/http/curl/curl.cpp Outdated Show resolved Hide resolved
sdk/core/azure-core/src/http/curl/curl.cpp Outdated Show resolved Hide resolved
@LarryOsterman LarryOsterman dismissed their stale review May 15, 2024 18:41

Waiting on Anton's approval.

@ahsonkhan ahsonkhan requested a review from Jinming-Hu May 15, 2024 18:42
@RickWinter RickWinter dismissed their stale review May 15, 2024 18:44

Clearing since the intent of the change is different

Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

I don't understand why can't we change the scope of variables in the same PR. Before, they needed to have greater scope with the previous if arrangement. In this PR, we find it to be essential to rearrange if statements, and even change the logic, yet we don't think that moving the related variables closer into the scope meets the bar.

The same with the log message. We do think it is essential to put it, yet we don't care enough to put it in the logging policy which would cover both libcurl and WinHTTP cases, and the log message here will no longer be necessary, and it will only be adding additional noise to the logs. In the logging policy, we log HTTP verb, we log URL, we log headers, so why not log HTTP version?

I agree there is a difference between doing simple things in one PR, and having more complex things in a follow-up PR. Here, I don't feel like I am proposing too much extra work to be done for this PR to stay simple and scoped, and in terms of implementation time and cost either.

@ahsonkhan
Copy link
Member Author

ahsonkhan commented May 16, 2024

At this time, I don't want to grow the scope/intent of this PR beyond what I have proposed (nor touch other lines of code within the curl transport). It has been waiting for approval for a week now, and it doesn't merit discussions on other changes (even if they are considered small).

If there's no blocking feedback on correctness, I would like to get this checked-in as is.

@antkmsft
Copy link
Member

Here I have the change that I am talking about, you can see it is in the same scope and does not increase it, and it is not much more code: ahsonkhan#2

@ahsonkhan ahsonkhan changed the title Harden the HTTP versions check in response parsing, validating specifically for 1.1 or 1.0, and throwing otherwise. Clean-up the HTTP versions check in response parsing, and don't reuse connections on any response other than HTTP/1.1 May 16, 2024
@ahsonkhan ahsonkhan enabled auto-merge (squash) May 16, 2024 22:02
@RickWinter
Copy link
Member

Here I have the change that I am talking about, you can see it is in the same scope and does not increase it, and it is not much more code: ahsonkhan#2

I like moving the parsing to where its needed. Would bring a slight improvement to perf since in the 1.1 we don't need to parse both headers.

The clean output of the httpVersion in the log is really nice bonus :)

Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

Ok, let's merge it, and I can create my PR on top of main instead of your PR - doesn't really matter that much how the change gets there.

@ahsonkhan ahsonkhan merged commit f517aab into Azure:main May 20, 2024
41 checks passed
@ahsonkhan ahsonkhan deleted the FixVersionCheck branch May 20, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants