Skip to content

Conversation

@teo-tsirpanis
Copy link
Contributor

Issue #, if available:

Description of changes:

This PR removes build-time detection of WinHTTP and WinINet support for HTTP/2 and HTTP/3. In its place, we always attempt setting the HTTP version, ignoring errors (on WinHTTP in certin cases we try both enabling both HTTP/2 and 3, and then only 2).

The main reason for this is to make cross-compilation from x64 to ARM64 easier. Besides that, the built binaries will automatically take advantage of the capabilities of the system's HTTP stack even if they are built on an older machine.

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

#endif

DWORD ConvertHttpVersionToWinHttpVersion(const Aws::Http::Version version)
std::initializer_list<DWORD> GetWinHttpVersionsToTry(const Aws::Http::Version version)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that initializer_list works in such context (return by value from a method), it is probably an undefined behavior here.
I'd suggest returning a const reference to a local static const object, such as

const std::initializer_list<DWORD>& GetWinHttpVersionsToTry(const Aws::Http::Version version) {
...
    else if (version == Version::HTTP_VERSION_3)
    {
      static const std::initializer_list<DWORD> HTTP123{ WINHTTP_PROTOCOL_FLAG_HTTP3 | WINHTTP_PROTOCOL_FLAG_HTTP2, WINHTTP_PROTOCOL_FLAG_HTTP2};
      return HTTP123;
    }

or use some other approach.

Please check the cpp reference:

Copying a std::initializer_list does not copy the backing array of the corresponding initializer list.

https://en.cppreference.com/w/cpp/utility/initializer_list

see also: https://stackoverflow.com/questions/15286450/lifetime-of-a-stdinitializer-list-return-value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@SergeyRyabinin
Copy link
Contributor

Hi @teo-tsirpanis , thank you a lot for submitting this pull request.

While the intention is clear and very good, I have a concern about std::initializer_list usage.
Please check the code review comment.
Best regards,

@SergeyRyabinin SergeyRyabinin merged commit a837d31 into aws:main Nov 21, 2024
2 of 4 checks passed
@teo-tsirpanis teo-tsirpanis deleted the winhttp branch November 21, 2024 21:33
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