Enable HTTP persistence when using Session with Invoke-WebRequest and Invoke-RestMethod #19173

stevenebutler · 2023-02-18T05:14:44Z

PR Summary

This PR addresses 12764 by using persistent HTTP connections when Invoke-WebRequest uses the -Session variable. It does this by

tracking the HttpClient object in the saved WebRequestSession
monitoring properties in WebRequestSession and rebuilding the HttpClient if they are changed. This is done to ensure that existing scripts that might tweak parameters between invocations of IWR actually apply the changes so they will behave in a backwards compatible manner. When anything that impacts the HttpClient is changed, the persistent connection is abandoned by disposing the HttpClient and underlying HttpClientHandler and constructing a new one. A warning is logged when this occurs as it has a performance impact (though no worse than the existing behaviour).
To support the point 2 above, many switches were added as internal set-only properties to the WebRequestSession so changes between invocations can be detected and the handler rebuilt appropriately.

PR Context

This PR makes a considerable difference to performance when using Invoke-WebRequest or Invoke-RestMethod with sessions when multiple requests are made to the same host. This is particularly evident for HTTPS requests, as each HTTPS connection can take hundreds of milliseconds to establish.

This PR as written passes existing tests in WebCmdlets.Tests.ps1 and additional WebSession tests that verify the HttpClient is rebuilt when appropriate. This PR is submitted as WIP for feedback following advice on #12764

The PR is attempting to provide as much backwards compatibility as possible at the expense of the complexity of tracking how the HttpClient used in the WebSession was constructed and recreating the HttpClient in the case it would have been constructed differently between invocations. Some feedback on whether this is a reasonable approach would help shape the PR.

PR Checklist

stevenebutler · 2023-02-18T05:19:23Z

@microsoft-github-policy-service agree

…onnections-in-cmdlets

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/WebRequestSession.cs

CarloToso · 2023-02-18T14:41:05Z

Maybe you could do something like this for WebRequestSession.cs (it's just an example I think it can be done better)

        internal int MaxAutomaticRedirections
        {
            get => _maxAutomaticRedirections;
            set => _maxAutomaticRedirections = SetVariable(typeof(int), _maxAutomaticRedirections, value);
        }

        internal dynamic SetVariable(Type type, object oldValue, object newValue)
        {
            dynamic convertedOldValue = Convert.ChangeType(oldValue, type);
            dynamic convertedNewValue = Convert.ChangeType(newValue, type);
            if (convertedOldValue != convertedNewValue)
            {
                _changed = true;
                return convertedNewValue;
            }
            else
            {
                return convertedOldValue;
            }
        }

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

CarloToso · 2023-02-18T15:00:57Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/WebRequestSession.cs

-            // build the cookie jar
-            Cookies = new CookieContainer();
+            // Build the cookie jar
+            _cookies = new CookieContainer();


If I understand the code correctly you can revert _cookies -> Cookies (and the same for all the others in WebRequestSession())

You could, but it would go through a longer code path checking for change when we already know it's changed. Is that the preferred option?

I think @iSazonov will answer to this question

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebProxy.cs

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/WebRequestSession.cs

stevenebutler · 2023-02-18T23:00:02Z

Maybe you could do something like this for WebRequestSession.cs (it's just an example I think it can be done better)

        internal int MaxAutomaticRedirections
        {
            get => _maxAutomaticRedirections;
            set => _maxAutomaticRedirections = SetVariable(typeof(int), _maxAutomaticRedirections, value);
        }

        internal dynamic SetVariable(Type type, object oldValue, object newValue)
        {
            dynamic convertedOldValue = Convert.ChangeType(oldValue, type);
            dynamic convertedNewValue = Convert.ChangeType(newValue, type);
            if (convertedOldValue != convertedNewValue)
            {
                _changed = true;
                return convertedNewValue;
            }
            else
            {
                return convertedOldValue;
            }
        }

I used a couple of generics to do something similar. I'm not a fan of using dynamic since it turns off lots of type checking.

   private void SetClassVar<T>(ref T oldValue, T newValue) where T : class
        {
            if (oldValue == newValue)
            {
                return;
            }
            _changed = true;
            oldValue = newValue;
        }

        private void SetStructVar<T>(ref T oldValue, T newValue) where T : struct
        {
            if (oldValue.Equals(newValue))
            {
                return;
            }
            _changed = true;
            oldValue = newValue;
        }

Let me know if there's a better way. I wasn't able to use a single generic for both structs and classes but I'm not an expert in this area and someone else may have a better suggestion.

@CarloToso

Addresses review comment by @CarloToso

CarloToso · 2023-02-19T12:43:44Z

@stevenebutler you could look into adding some tests to WebCmdlets.Tests.ps1

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/WebRequestSession.cs

stevenebutler · 2023-02-19T20:45:15Z

You would either have to accept that persistence would not work if the user supplied any client handler impacting parameters to iwr or you would have to check every assignment to the handler for logical equivalence before attempting to set it with this approach. You could argue that to get persistence they should clean up the script parameters. I was going for maximum performance with least script change effort and maximum compatibility with this PR.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/WebRequestSession.cs

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebProxy.cs

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/WebRequestSession.cs

* Move GetHttpClient body to WebRequestSession * Make internal properties setter only * Implement IDisposable for WebRequestSession * Add tests for persistence * Fix GetHashCode() / Equals() on WebProxy

@iSazonov

As per review request from @iSazonov, WebRequestPSCmdlet implements IDisposable and cleans up non-persistent WebSession objects in the Dispose method.

@iSazonov

Responding to review comments from @iSazonov

CarloToso · 2023-02-26T00:36:02Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+                $caught = $true
+            }
+            $Warnings | Should -be $connWarning
+            $caught | should -BeTrue


Suggested change

$caught | should -BeTrue

$caught | Should -BeTrue

CarloToso · 2023-02-26T00:36:18Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+            } catch {
+                $caught = $true
+            }
+            $Warnings | Should -be $connWarning


Suggested change

$Warnings | Should -be $connWarning

$Warnings | Should -Be $connWarning

CarloToso · 2023-02-26T00:46:48Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+
+        It 'Connection persistence maintained' {
+            $Uri = Get-WebListenerUrl
+            $null = Invoke-WebRequest -Uri $uri -SessionVariable Session -wv warnings


Suggested change

$null = Invoke-WebRequest -Uri $uri -SessionVariable Session -wv warnings

$null = Invoke-WebRequest -Uri $uri -SessionVariable Session -WarningVariable Warnings

Could you please, always use variables with the same capitalization in the tests

CarloToso · 2023-02-26T01:24:42Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+        }
+
+        It 'Connection persistence outperforms non-persistence' {
+            $Uri = Get-WebListenerUrl -Https


Suggested change

$Uri = Get-WebListenerUrl -Https

$uri = Get-WebListenerUrl -Https

I have addressed the naming issues in the latest commit.

I think you can remove the W.I.P label

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

@iSazonov

As per review request by @iSazonov

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

iSazonov · 2023-02-28T07:08:24Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

+                    {
+                        // If both ProxyCredential and ProxyUseDefaultCredentials are passed,
+                        // UseDefaultCredentials will overwrite the supplied credentials.
+                        webProxy.UseDefaultCredentials = true;


The comment is not correct since the code is in else branch.

Yes, you are right. However this is what was there before, which makes me wonder if it's a bug, or just a bad comment?

Should we fix the comment or the code?

@CarloToso Could you please look this?

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

iSazonov · 2023-02-28T07:30:41Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+            $null = Invoke-WebRequest $uri -WebSession $Session -Proxy 'http://localhost:8080' -WarningVariable warnings
+            $warnings | Should -Be $connWarning
+        }
+    }
 }

 Describe "Invoke-RestMethod tests" -Tags "Feature", "RequireAdminOnWindows" {


We have to duplicate the tests for Invoke-RestMethod cmdlet.

Let's get it right for IWR then I'll look at duplicating.

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

Co-authored-by: Ilya <darpa@yandex.ru>

Comment improvements Changed reconnection warning to verbose message Moved verbose message text to resource Updated tests

CarloToso · 2023-03-01T00:46:32Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/WebRequestSession.cs

-            // This indicates GetResponse will handle redirects.
-            SetStructVar(ref _allowAutoRedirect, !(handleRedirect || MaximumRedirection == 0));
+            // Do not auto redirect if the the caller does not want it, or maximum redirections is 0
+            SetStructVar(ref _allowAutoRedirect, !(doNotRedirect || MaximumRedirection == 0));


I think the change from handleRedirect to doNotRedirect or doNotHandleRedirect could be confusing. Maybe you can find a new name that always makes sense. Maybe cmdletHandleRedirect or something better

It is handleRedirect from the perspective of cmdlet, but do not from the perspective of the websession's httpclient. I'm not sure what else to call it. Have made the change you suggested to doNotHandleRedirect in the WebSession.

I think doNotHandleRedirect is better but it's still not the best (sorry I deleted the suggestion, I was thinking of a more fitting name), let's keep thinking about it and use doNotHandleRedirect in the meantime

How about supressHttpClientRedirects ?

Also, could you take a look at @iSazonov 's comment regarding the comment around UseDefaultCredentials. It looks wrong based on the code, but I'm not sure if the code or comment is ultimately correct.

// If both ProxyCredential and ProxyUseDefaultCredentials are passed, // UseDefaultCredentials will overwrite the supplied credentials. webProxy.UseDefaultCredentials = true;

I gave it a quick look and it seems the code could be wrong, not only there but every time we use UseDefaultCredentials, when I have some time I will open a tracking issue to investigate.

supressHttpClientRedirects might work, what do you think @iSazonov?

Maybe more short disableRedirect.

pull-request-quantifier-deprecated · 2023-03-01T01:50:43Z

This PR has 338 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Large
Size       : +282 -56
Percentile : 73.8%

Total files changed: 5

Change summary by file extension:
.cs : +149 -56
.resx : +3 -0
.ps1 : +130 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

iSazonov · 2023-03-01T05:08:37Z

@stevenebutler The PR contains too many commits and comments. I suggest to close the PR and open new one. Also maybe squash commits - it seems the history is not so useful.

stevenebutler · 2023-03-01T11:14:11Z

I am closing this PR in favour of #19249

Enable HTTP persistence when using Session

a7f576e

stevenebutler requested a review from PaulHigin as a code owner February 18, 2023 05:14

pull-request-quantifier-deprecated bot added the Medium label Feb 18, 2023

ghost assigned adityapatwardhan Feb 18, 2023

stevenebutler and others added 2 commits February 18, 2023 15:35

Address codefactor issues

c5addae

Merge branch 'PowerShell:master' into 12764-support-persistent-http-c…

ac534ef

…onnections-in-cmdlets

CarloToso reviewed Feb 18, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/WebRequestSession.cs Outdated Show resolved Hide resolved

CarloToso reviewed Feb 18, 2023

View reviewed changes

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated Show resolved Hide resolved

CarloToso reviewed Feb 18, 2023

View reviewed changes

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated Show resolved Hide resolved

CarloToso reviewed Feb 18, 2023

View reviewed changes

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated Show resolved Hide resolved

CarloToso reviewed Feb 18, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebProxy.cs Outdated Show resolved Hide resolved

CarloToso reviewed Feb 18, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebProxy.cs Show resolved Hide resolved

CarloToso reviewed Feb 18, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/WebRequestSession.cs Outdated Show resolved Hide resolved

stevenebutler added 3 commits February 19, 2023 09:28

Address review comments by @CarloToso

c722d2c

Make -NoProxy and -Proxy switches flip each other's states

08e96a1

Addresses review comment by @CarloToso

Address codefactor issues

9dda627

CarloToso reviewed Feb 19, 2023

View reviewed changes

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated Show resolved Hide resolved

iSazonov reviewed Feb 19, 2023

View reviewed changes

iSazonov reviewed Feb 21, 2023

View reviewed changes

CarloToso mentioned this pull request Feb 23, 2023

WebCmdlets Investigation on the usage of webProxy.BypassProxyOnLocal #19203

Closed

Address @iSazanov review comments and suggestions

7797a91

* Move GetHttpClient body to WebRequestSession * Make internal properties setter only * Implement IDisposable for WebRequestSession * Add tests for persistence * Fix GetHashCode() / Equals() on WebProxy

pull-request-quantifier-deprecated bot added Large and removed Medium labels Feb 25, 2023

stevenebutler added 2 commits February 26, 2023 07:23

Implements IDisposable to dispose WebSession

1c7c0fd

As per review request from @iSazonov, WebRequestPSCmdlet implements IDisposable and cleans up non-persistent WebSession objects in the Dispose method.

Move TimeoutSec handling to WebRequestSession

b69fe72

Responding to review comments from @iSazonov

CarloToso reviewed Feb 26, 2023

View reviewed changes

Clean up variable naming in persistence test cases

18eb46e

iSazonov reviewed Feb 26, 2023

View reviewed changes

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated Show resolved Hide resolved

Combine if expressions in dispose

6bb5283

As per review request by @iSazonov

CarloToso mentioned this pull request Feb 26, 2023

WebRequestPSCmdlet.Common.cs remove { } and indentation #19229

Merged

22 tasks

stevenebutler changed the title ~~WIP: Enable HTTP persistence when using Session with Invoke-WebRequest and Invoke-RestMethod~~ Enable HTTP persistence when using Session with Invoke-WebRequest and Invoke-RestMethod Feb 26, 2023

stevenebutler added 2 commits February 28, 2023 09:30

Remove indentation

7ae5471

Merge indentation change from master

b7a5aff

iSazonov reviewed Feb 28, 2023

View reviewed changes

iSazonov mentioned this pull request Feb 28, 2023

Small style changes #19241

Merged

22 tasks

CarloToso added a commit to CarloToso/PowerShell that referenced this pull request Feb 28, 2023

Move style changes from PowerShell#19173

db39377

stevenebutler and others added 4 commits March 1, 2023 07:26

Apply suggestions from code review

2aee3d8

Co-authored-by: Ilya <darpa@yandex.ru>

Address review comments by @iSazonov

d747ea2

Comment improvements Changed reconnection warning to verbose message Moved verbose message text to resource Updated tests

codefactor fix

237aaf7

Make parameter name clearer as per review suggestion

14487d4

CarloToso reviewed Mar 1, 2023

View reviewed changes

Rename function parameter to suppressHttpClientRedirects

9f73991

stevenebutler closed this Mar 1, 2023

This was referenced Mar 1, 2023

Remove comment and simplify condition in WebCmdlets #19251

Merged

Support HTTP persistent connections in Web Cmdlets #19249

Merged

stevenebutler deleted the 12764-support-persistent-http-connections-in-cmdlets branch March 11, 2023 04:28

CarloToso mentioned this pull request Mar 15, 2023

WebCmdlets document ConnectionPooling with -SessionVariable then -WebSession MicrosoftDocs/PowerShell-Docs#9914

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable HTTP persistence when using Session with Invoke-WebRequest and Invoke-RestMethod #19173

Enable HTTP persistence when using Session with Invoke-WebRequest and Invoke-RestMethod #19173

stevenebutler commented Feb 18, 2023 •

edited

stevenebutler commented Feb 18, 2023

CarloToso commented Feb 18, 2023 •

edited

CarloToso Feb 18, 2023

stevenebutler Feb 18, 2023

CarloToso Feb 19, 2023

stevenebutler commented Feb 18, 2023

CarloToso commented Feb 19, 2023

stevenebutler commented Feb 19, 2023 via email •

edited by ghost

CarloToso Feb 26, 2023

CarloToso Feb 26, 2023

CarloToso Feb 26, 2023 •

edited

CarloToso Feb 26, 2023

stevenebutler Feb 26, 2023

CarloToso Feb 26, 2023

iSazonov Feb 28, 2023

stevenebutler Feb 28, 2023

iSazonov Mar 1, 2023

iSazonov Feb 28, 2023

stevenebutler Feb 28, 2023

CarloToso Mar 1, 2023 •

edited

stevenebutler Mar 1, 2023

CarloToso Mar 1, 2023 •

edited

stevenebutler Mar 1, 2023 •

edited

CarloToso Mar 1, 2023

iSazonov Mar 1, 2023

pull-request-quantifier-deprecated bot commented Mar 1, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

iSazonov commented Mar 1, 2023

stevenebutler commented Mar 1, 2023

	$Warnings \| Should -be $connWarning
	$Warnings \| Should -Be $connWarning

	$null = Invoke-WebRequest -Uri $uri -SessionVariable Session -wv warnings
	$null = Invoke-WebRequest -Uri $uri -SessionVariable Session -WarningVariable Warnings

	$Uri = Get-WebListenerUrl -Https
	$uri = Get-WebListenerUrl -Https

Enable HTTP persistence when using Session with Invoke-WebRequest and Invoke-RestMethod #19173

Enable HTTP persistence when using Session with Invoke-WebRequest and Invoke-RestMethod #19173

Conversation

stevenebutler commented Feb 18, 2023 • edited

PR Summary

PR Context

PR Checklist

stevenebutler commented Feb 18, 2023

CarloToso commented Feb 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenebutler commented Feb 18, 2023

CarloToso commented Feb 19, 2023

stevenebutler commented Feb 19, 2023 via email • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarloToso Feb 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarloToso Mar 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarloToso Mar 1, 2023 • edited

Choose a reason for hiding this comment

stevenebutler Mar 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Mar 1, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

iSazonov commented Mar 1, 2023

stevenebutler commented Mar 1, 2023

stevenebutler commented Feb 18, 2023 •

edited

CarloToso commented Feb 18, 2023 •

edited

stevenebutler commented Feb 19, 2023 via email •

edited by ghost

CarloToso Feb 26, 2023 •

edited

CarloToso Mar 1, 2023 •

edited

CarloToso Mar 1, 2023 •

edited

stevenebutler Mar 1, 2023 •

edited