Microsoft.PowerShell.Commands.WebProxy replace System.Net.IWebProxy with System.Net.WebProxy #19609

CarloToso · 2023-05-03T18:40:08Z

PR Summary

It seems we can't remove Microsoft.PowerShell.Commands.WebProxy because we need to implement IEquatable<WebProxy>

Replaces System.Net.IWebProxy with System.Net.WebProxy

PR Context

Discussed with @iSazonov in another PR #19359 (comment)

PR Checklist

iSazonov · 2023-05-04T03:41:51Z

@CarloToso Please resolve merge conflicts.

CarloToso · 2023-05-04T08:37:24Z

@stevenebutler could you help me with the failing test?

stevenebutler · 2023-05-05T22:06:37Z

@stevenebutler could you help me with the failing test?

With the change made I would expect the test to fail.

The test is ensuring that supplying the Proxy parameter only causes a new connection to be created if the new proxy is different to the old proxy because changing the proxy is not permitted once a request has been issued.

If you don't use the pwsh WebProxy class with the Equality operator defined then the equality check will always fail so a new connection will be used regardless of providing the same Proxy parameter.

You can trade-off the performance impact of creating a new connection and change the test or implement another way to avoid recreating the HttpClientHandler if the same proxy is repeatedly provided on the command line.

Why did pwsh have its own WebProxy class in the first place, and why do we want to get rid of it now?

CarloToso · 2023-05-05T22:44:02Z

Thank you very much for your explanation.
It seems pwsh own WebProxy class it was introduced in 2016 46f8e0d. I think it was necessary because dotnet core didn't have a WebProxy class yet.
I want to remove the WebProxy class (if it's possible) to cleanup old code.

stevenebutler · 2023-05-05T22:46:22Z

Thank you very much for your explanation. It seems pwsh own WebProxy class it was introduced in 2016 46f8e0d. I think it was necessary because dotnet core didn't have a WebProxy class yet. I want to remove the WebProxy class (if it's possible) to cleanup old code.

You can probably re-implement the point of the comparison Equals operator in the WebProxy class I added directly into the body of the method. i.e. only create a new WebProxy if the Proxy is provided on the command line and the proxy string is different to the one stored in the existing WebProxy in the WebSession (assuming there is one of course). I'm assuming you are able to retrieve that value from the system WebProxy through a property. If you can do that the test should pass.

This may be a minor breaking change if anyone is checking the class name of the existing webproxy or if it has properties that don't exist on the system one that they're using to drive logic in their scripts. I can't imagine why anybody would want to do that though.

CarloToso · 2023-05-05T22:48:33Z

Thank you, I'll look into it

iSazonov · 2023-05-07T16:02:39Z

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

 namespace Microsoft.PowerShell.Commands
 {
-    internal class WebProxy : IWebProxy, IEquatable<WebProxy>
+    internal class WebProxy : System.Net.WebProxy, IEquatable<WebProxy>


I would assume that reference equality is enough to make the cache work for most real-world scenarios. Then we could still remove this type.

Using ReferenceEquals does not work, the tests fail

I think it is still possible to remove the class - the Address property on System.Net.WebProxy is available and could be used to compare with the passed in Proxy parameter (if one is provided).

The Address property on System.Net.WebProxy is also settable, and can be changed without causing exceptions between requests from HttpClient so it may be possible to simply update the Address property on the existing Proxy provided:

the WebSession already has a Proxy (i.e. this is not a brand new WebSession)

the type of the Proxy is System.Net.WebProxy

Updating the proxy Address property does cause a new connection to be made (to connect to the new proxy), but this doesn't require creation of a new HttpClientHandler.

Note that no new connection is required if the user updates the Address property to the same value it already had.

Using ReferenceEquals does not work, the tests fail

I don't see the commit.

I tested in in local

I see no reason why it wouldn't work. Unless there happens to be wrapping in PSObject which can be bypassed.

It doesn't work because there is a test that checks that passing the same -Proxy repeatedly with a persistent websession doesn't cause a new httpclienthandler to be created. If you use referenceequals then this will not be true.

The code from the Equals() could be moved into the CmdLet if you want to retain the existing behaviour but remove the internal WebProxy class completely.

Also note that you can modify properties on the WebProxy without impacting the HttpClientHandler so there's no need to really check all the properties. The only thing that really makes a difference is the Proxy Address.

pull-request-quantifier-deprecated · 2023-05-08T20:19:30Z

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

Quantification details

Label      : Small
Size       : +19 -32
Percentile : 20.4%

Total files changed: 2

Change summary by file extension:
.cs : +19 -32

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-05-09T17:30:19Z

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

+                // We don't want to update the WebSession unless the proxies are different
+                // as that will require us to create a new HttpClientHandler and lose connection
+                // persistence.
+                if (!webProxy.Equals(WebSession.Proxy))


Really we use 3 parameters to create webProxy. So we could keep them in WebSession
to track WebSession.Proxy changes. This also exclude creating proxy in line 963 if no need.

At simplest level, couldn't the comparison be something like this (excuse the possible namespace confusion, I've typed this directly into GitHub comment uncompiled/untested)?

if (!string.IsNullOrEmpty(Proxy) && Proxy != (WebSession?.Proxy as System.Net.WebProxy)?.Address) { if (WebSession.Proxy is System.Net.WebProxy wsProxy) { wsProxy.Address = Proxy; } else { // Proxy type is unexpected so replace with standard proxy (since the user supplied the Proxy parameter) WebSession.Proxy = new System.Net.WebProxy(Proxy, true); } }

This may not pass the existing tests because reassigning Address from Proxy parameter when they're different won't cause the HttpClient to be recreated, but it should work, and that test's expected result could be updated accordingly.

If you're also trying to deal with the BypassProxyOnLocal parameter (did you add that to the CmdLet because there's no way in current PowerShell to affect that setting), you'll need some extra logic for that too, but it can also be modified
directly on the Proxy class without needing to recreate it (provided the stored WebSession.Proxy is a System.Net.WebProxy object and not some custom Proxy class designed by the user).

Edit: I just noticed I used Address instead of Proxy in the new System.Net.WebProxy(Proxy, true); (third last line)

ghost · 2023-05-17T14:00:55Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

JamesWTruher · 2023-12-04T18:35:16Z

@iSazonov have your questions/concerns been satisfied?

iSazonov · 2023-12-05T03:33:33Z

@JamesWTruher

As I see it, this PR has not been completed. We need to delete the internal Web Proxy class and for this to correctly track changes to the argument in the Proxy parameter. There's a suggestion from @stevenebutler on how to do it.

@CarloToso Do you want to continue?

CarloToso · 2023-12-11T20:59:54Z

@JamesWTruher

As I see it, this PR has not been completed. We need to delete the internal Web Proxy class and for this to correctly track changes to the argument in the Proxy parameter. There's a suggestion from @stevenebutler on how to do it.

@CarloToso Do you want to continue?

I'll try to come back to this PR as soon as I have some spare time

Replace Microsoft.PowerShell.Commands.WebProxy with System.Net.WebProxy

fb22f7f

CarloToso requested a review from PaulHigin as a code owner May 3, 2023 18:40

pull-request-quantifier-deprecated bot added the Extra Small label May 3, 2023

ghost assigned iSazonov May 3, 2023

CarloToso closed this May 3, 2023

CarloToso reopened this May 3, 2023

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label May 4, 2023

Merge branch 'master' into proxy

dc452cc

add back Microsoft.PowerShell.Commands.WebProxy

4375138

CarloToso changed the title ~~Replace Microsoft.PowerShell.Commands.WebProxy with System.Net.WebProxy~~ Microsoft.PowerShell.Commands.WebProxy replace System.Net.IWebProxy with System.Net.WebProxy May 6, 2023

use == operator for Address

f474bca

iSazonov reviewed May 7, 2023

View reviewed changes

More in depth WebProxy comparison

25c6ef8

StevenBucher98 added the PowerShell-Docs needed The PR was reviewed and a PowerShell Docs update is needed label May 8, 2023

CarloToso added 2 commits May 8, 2023 19:41

more in depth equality fix; cleanup nested if

bebe5b3

compare BypassList

8be91a2

pull-request-quantifier-deprecated bot added Small and removed Extra Small labels May 8, 2023

Merge branch 'PowerShell:master' into proxy

adbafe8

CarloToso mentioned this pull request May 9, 2023

WebCmdlets Investigation on the usage of webProxy.BypassProxyOnLocal #19203

Closed

iSazonov reviewed May 9, 2023

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label May 17, 2023

SteveL-MSFT added the CommunityDay-Small A small PR that the PS team has identified to prioritize to review label Jul 24, 2023

microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Dec 5, 2023

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Dec 19, 2023

daxian-dbw added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed CommunityDay-Small A small PR that the PS team has identified to prioritize to review labels Feb 12, 2024

microsoft-github-policy-service bot added Stale and removed Stale labels Feb 27, 2024

microsoft-github-policy-service bot added the Stale label Mar 14, 2024

microsoft-github-policy-service bot closed this Mar 24, 2024

microsoft-github-policy-service bot removed the Stale label Mar 24, 2024

Microsoft.PowerShell.Commands.WebProxy replace System.Net.IWebProxy with System.Net.WebProxy #19609

Microsoft.PowerShell.Commands.WebProxy replace System.Net.IWebProxy with System.Net.WebProxy #19609

Uh oh!

Conversation

CarloToso commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

iSazonov commented May 4, 2023

Uh oh!

CarloToso commented May 4, 2023

Uh oh!

stevenebutler commented May 5, 2023

Uh oh!

CarloToso commented May 5, 2023

Uh oh!

stevenebutler commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarloToso commented May 5, 2023

Uh oh!

iSazonov May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarloToso May 7, 2023

Choose a reason for hiding this comment

Uh oh!

stevenebutler May 7, 2023

Choose a reason for hiding this comment

Uh oh!

iSazonov May 8, 2023

Choose a reason for hiding this comment

Uh oh!

CarloToso May 8, 2023

Choose a reason for hiding this comment

Uh oh!

iSazonov May 8, 2023

Choose a reason for hiding this comment

Uh oh!

stevenebutler May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pull-request-quantifier-deprecated bot commented May 8, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

iSazonov May 9, 2023

Choose a reason for hiding this comment

Uh oh!

stevenebutler May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 17, 2023

Uh oh!

JamesWTruher commented Dec 4, 2023

Uh oh!

iSazonov commented Dec 5, 2023

Uh oh!

CarloToso commented Dec 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CarloToso commented May 3, 2023 •

edited

Loading

stevenebutler commented May 5, 2023 •

edited

Loading

iSazonov May 7, 2023 •

edited

Loading

stevenebutler May 8, 2023 •

edited

Loading

stevenebutler May 9, 2023 •

edited

Loading