Add WinHTTP Transport support to the SDK for windows clients as another HTTP Transport. by ahsonkhan · Pull Request #897 · Azure/azure-sdk-for-cpp

ahsonkhan · 2020-11-04T17:27:36Z

Fixes #354

…er HTTP Transport.

vhvb1989

Changelog
Readme docs about how to build and use

Really recommending to split the code from send() into multiple components. It is currently creating the handles, sending request , reading/parsing response and creating rawResponse.

The handles are copied to the bodyStream. Add a destructor for the bodyStream to clean the handles. Only the Send() method is caring about CleanupHandlesAndThrow, but if Read() fails or if the bodyStream is disposed, are we leaking?

Create an abstraction for writing/reading from the wire with an interface. Then create a derive class to implement the interface. This will let you create a mocked derived object to emulate the server's responses.

ahsonkhan · 2020-11-05T19:24:59Z

The handles are copied to the bodyStream. Add a destructor for the bodyStream to clean the handles. Only the Send() method is caring about CleanupHandlesAndThrow, but if Read() fails or if the bodyStream is disposed, are we leaking?

That is already happening. We are not leaking :)

See:

azure-sdk-for-cpp/sdk/core/azure-core/inc/azure/core/http/winhttp/win_http_client.hpp

Lines 60 to 65 in 16bef10

    
           ~WinHttpStream() override 
        
           { 
        
             WinHttpCloseHandle(this->m_requestHandle); 
        
             WinHttpCloseHandle(this->m_connectionHandle); 
        
             WinHttpCloseHandle(this->m_sessionHandle); 
        
           }

This reverts commit d44253f.

check-enforcer · 2020-12-05T00:43:30Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run cpp - [service] - ci

… into WinHttpTransport

…ader dependencies.

ahsonkhan · 2020-12-05T02:25:17Z

/azp run cpp - storage

azure-pipelines · 2020-12-05T02:25:28Z

Azure Pipelines successfully started running 1 pipeline(s).

update CI pipeline.

module.

azure-pipelines · 2020-12-09T20:44:07Z

Azure Pipelines successfully started running 1 pipeline(s).

vhvb1989

Adding a few comments.

Feel free to create issues to address those things in the future if you are ready to merge.

vhvb1989 · 2020-12-09T20:19:25Z

  private:
+    WinHttpTransportOptions m_options;
+
+    void GetSessionHandle(std::unique_ptr<Details::HandleManager>& handleManager);


I don't know if it would just better to use a pointer here like Details::HandleManager const* const or Details::HandleManager * const.
The main purpose of using the unique_ptr is to handle the ownership and ensure only only owner. My recommendation is to change this signatures to a raw pointer. We can still keep a unique_ptr wherever it's need, We can get the raw pointer from the unique_ptr by calling .get().
@antkmsft , I would like to hear from you here since you know better understanding of this

vhvb1989 · 2020-12-09T23:24:26Z

+  GetSessionHandle(handleManager);
+  GetConnectionHandle(handleManager);
+  GetRequestHandle(handleManager);
+
+  SendRequest(handleManager);
+
+  ReceiveResponse(handleManager);
+
+  return GetRawResponse(handleManager, request.GetMethod());


All these methods dont's need to get a unique_ptr.
Instead, they can just take a referece or a raw pointer to HandleManager.

The benefit, if it just a reference, is that you never worry about null argument exception.
See, I made this example on how to pass a reference from a unique_ptr, I recommend following this approach here
https://godbolt.org/z/71G7nd

Given this is an implementation detail and seems somewhat subjective, I am inclined to leave it as is for now.

One thing I need is the destructor of handleManager to be called whenever we throw. Would passing them as raw pointers or a reference still let that happen?

update AddHeader docs, and remove TODOs in source.

… into WinHttpTransport

…ious signature with pointers.

AddHeader method that takes iterators.

ahsonkhan · 2020-12-10T03:29:43Z

/azp run cpp - storage

azure-pipelines · 2020-12-10T03:29:53Z

Azure Pipelines successfully started running 1 pipeline(s).

ahsonkhan · 2020-12-10T04:00:45Z

/azp run cpp - storage

azure-pipelines · 2020-12-10T04:00:55Z

Azure Pipelines successfully started running 1 pipeline(s).

vhvb1989

Adding some responses

Also mentioning an issue with the unique_ptr that needs to be fixed before merging.
The WinHttpBodyStream is currently stealing the handleManager ownership instead of expecting to become the owner

vhvb1989 · 2020-12-10T06:20:53Z

+  // Specify an HTTP server.
+  // Uses port 80 for HTTP and port 443 for HTTPS.
+  // This function always operates synchronously.
+  handleManager->m_connectionHandle = WinHttpConnect(


It's not managing anything. It is acting just as a container or bag. But any name you decide is up to up as the author.

ahsonkhan · 2020-12-10T21:16:59Z

/azp run cpp - storage

azure-pipelines · 2020-12-10T21:17:11Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2020-12-10T21:18:12Z

Hello @ahsonkhan!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ahsonkhan · 2020-12-10T21:32:28Z

Thanks for the great and detailed feedback @vhvb1989. Appreciate it!

If anyone else has any other pressing feedback or wants me to change something, please review it and let me know and I will address it separately. In the interest of time, given the duration and size of this PR and conversation, I am going to merge this as-is, once CI is green (again).

ahsonkhan · 2020-12-10T22:49:25Z

@Jinming-Hu can't wait for you to share the performance numbers on Windows. I have some optimizations in mind that I suspect can get us some improvements, but wanted to see the change first to get a sense of order-of-magnitude. Thanks!

Add WinHTTP Transport support to the SDK for windows clients as anoth…

16bef10

…er HTTP Transport.

ahsonkhan added feature-request This issue requires a new behavior in the product in order be resolved. Azure.Core Client This issue points to a problem in the data-plane of the library. labels Nov 4, 2020

ahsonkhan added this to the [2020] November milestone Nov 4, 2020

ahsonkhan self-assigned this Nov 4, 2020

RickWinter reviewed Nov 4, 2020

View reviewed changes

Comment thread sdk/core/azure-core/test/ut/transport_adapter.cpp Outdated

RickWinter reviewed Nov 4, 2020

View reviewed changes

Comment thread sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp Outdated

vhvb1989 reviewed Nov 5, 2020

View reviewed changes

This was referenced Nov 9, 2020

Only link the relevant transport library when that component is being built #926

Closed

Investigate the best buffer size strategy for WinHTTP when uploading a request body and a user-defined size isn't provided #927

Closed

Address PR feedback.

257e73a

RickWinter modified the milestones: [2020] November, [2020] December Nov 18, 2020

ahsonkhan added 5 commits November 19, 2020 17:12

Add a session handle field to the transport for cleanup.

0a12b5d

Use winhttp in blob client test.

ca9a194

Fix reason phrase size and make sure to pass query parameters

2461da2

Use WinHttp instead of Curl for testing.

d44253f

Revert "Use WinHttp instead of Curl for testing."

ae93dc0

This reverts commit d44253f.

ahsonkhan and others added 3 commits December 4, 2020 16:44

Merge branch 'master' of https://github.com/ahsonkhan/azure-sdk-for-cpp…

b89b3f4

… into WinHttpTransport

Use WinHttp as the default transport policy for testing.

3e3f9f3

Update header to avoid using policy.hpp since it results in cyclic he…

7bece46

…ader dependencies.

ahsonkhan added 4 commits December 7, 2020 19:20

Add win http transport adapter tests, fix issues with Head methd, and

1716587

update CI pipeline.

Update default policy being used and fix the transport adapter cmake

a914d39

module.

Fix spacing and get rid of unnecessary header includes in test adapter.

0035dd1

Fix use of #elif directive.

3e4d9a7

vhvb1989 approved these changes Dec 9, 2020

View reviewed changes

ahsonkhan added 4 commits December 9, 2020 19:00

Address PR feedback - user iterators, rename private methods to Create,

059883c

update AddHeader docs, and remove TODOs in source.

Merge branch 'master' of https://github.com/ahsonkhan/azure-sdk-for-cpp…

20e7dad

… into WinHttpTransport

Remove unnecessary qualification on AddHeader and revert back to prev…

e750b3c

…ious signature with pointers.

Revert to use the simpler pointer approach since we don't have an

e3aa22a

AddHeader method that takes iterators.

Add back re-interpret casts to call the correct AddHeader overload.

64e50ee

vhvb1989 approved these changes Dec 10, 2020

View reviewed changes

RickWinter modified the milestones: [2020] December, [2021] January Dec 10, 2020

Address PR feedback - explicitly pass ownership to bodystream

3689f83

ahsonkhan added the auto-merge label Dec 10, 2020

ghost merged commit 0f854ff into Azure:master Dec 10, 2020

ahsonkhan deleted the WinHttpTransport branch December 10, 2020 22:47

ahsonkhan mentioned this pull request Jan 13, 2021

Update the changelog wording and content for the upcoming release. #1340

Merged

This pull request was closed.

Conversation

ahsonkhan commented Nov 4, 2020

Uh oh!

Uh oh!

Uh oh!

vhvb1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

check-enforcer Bot commented Dec 5, 2020

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

ahsonkhan commented Dec 5, 2020

Uh oh!

azure-pipelines Bot commented Dec 5, 2020

Uh oh!

azure-pipelines Bot commented Dec 9, 2020

Uh oh!

vhvb1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vhvb1989 Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

vhvb1989 Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan commented Dec 10, 2020

Uh oh!

azure-pipelines Bot commented Dec 10, 2020

Uh oh!

ahsonkhan commented Dec 10, 2020

Uh oh!

azure-pipelines Bot commented Dec 10, 2020

Uh oh!

vhvb1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vhvb1989 Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan commented Dec 10, 2020

Uh oh!

azure-pipelines Bot commented Dec 10, 2020

Uh oh!

ahsonkhan commented Nov 5, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.