Invoke-WebRequest and Invoke-RestMethod: Rename TimeoutSec to ConnectionTimeoutSeconds (with alias) and add OperationTimeoutSeconds

[stevenebutler]

PR Summary

This renames -TimeoutSec parameter (with alias to old parameter name) to -ConnectionTimeoutSeconds and adds -OperationTimeoutSeconds parameter to both Invoke-WebRequest and Invoke-RestMethod commands.

• -TimeoutSec becomes -ConnectionTimeoutSeconds and is aliased with -TimeoutSec and has same behaviour as before. That is, it is the timeout that applies from just prior to sending a web request to receiving the response headers.
• -OperationTimeoutSeconds specifies the maximum allowed time between reads from the network when streaming http response content.

The default for both items is to have no timeout.

close #12249
close #16122
close #19519

PR Context

There are reports that when a network stream goes away and the OS doesn't or is unable to notify PowerShell that the network stream has been lost that PowerShell hangs forever in Invoke-WebRequest / Invoke-RestMethod on stalled response streams.

A previous PR made it possible to use CTRL-C to terminate this, but there was no way to have PowerShell timeout after a period of network inactivity in batch scenarios. This PR adds the timeout. The timeout applies to inter-stream reads, and not to the stream time as a whole. Therefore setting the -OperationTimeoutSeconds to 30 seconds means that any delay of longer than 30 seconds between reading data from the stream will terminate the request. However a large file that takes several minutes to download will not terminate unless the stream stalls for more than 30 seconds.

The default of 0 for -ConnectionTimeoutSeconds and -OperationTimeoutSeconds means no timeout applies as per request by WG noted in #19519. Any value less than 0 for -OperationTimeoutSeconds is also treated as no timeout.

Performance Comparison

The following benchmark was used to assess performance implications of this PR over 7.3.4 vanilla using a local dotnet minimal API with a static array of bytes being returned by the website (source below).

All tests were run on a HP laptop with 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz 2.70 GHz and 16 GB memory. Note the dotnet web server and the client application were tested over HTTP on the same laptop over localhost interface.

Note: testing was done on an earlier iteration before renaming and minor refactoring of timeout handling.

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

var bytes = new byte[200*1024*1024]; // ~200 MB array
Random.Shared.NextBytes(bytes);
Console.WriteLine("Byte stream initialized");
app.MapGet("/", () => Results.Bytes(bytes));

app.Run();

The benchmark web server was built in Release configuration and run as an executable.

# Curl from 7.3.4 vanilla
1 .. 10 | %{(Measure-Command { curl http://localhost:5000/ --output curl_data.bin }).TotalMilliseconds} | Measure-Object -Average -Minimum -Maximum -StandardDeviation
# Command run twice, and second summary taken
Count             : 10
Average           : 733.3488
Maximum           : 879.6177
Minimum           : 632.9115
StandardDeviation : 69.7117181472224

# PowerShell 7.3.4 vanilla
1 .. 10 | %{(Measure-Command { iwr http://localhost:5000/ -OutFile iwr_7.3.4_data.bin }).TotalMilliseconds} | Measure-Object -Average -Minimum -Maximum -StandardDeviation

# Command run twice and second summary taken
Count             : 10
Average           : 1115.43297
Sum               :
Maximum           : 1132.2394
Minimum           : 1102.5451
StandardDeviation : 11.4286089899233

# Powershell 7.4.0-preview.3
1 .. 10 | %{(Measure-Command { iwr http://localhost:5000/ -OutFile iwr_7.4.0-preview-3_data.bin }).TotalMilliseconds} | Measure-Object -Average -Minimum -Maximum -StandardDeviation

# Command run twice and second summary taken 
Count             : 10
Average           : 415.06509
Sum               :
Maximum           : 474.9918
Minimum           : 388.273
StandardDeviation : 33.2625053366048
Property          :

# Powershell 7.4.0-preview.3-81-g6b80bf03b74dff76816740fbb3549b87f190f518
1 .. 10 | %{(Measure-Command { iwr http://localhost:5000/ -OutFile iwr_7.4.0-preview.3-with-19558_data.bin }).TotalMilliseconds} | Measure-Object -Average -Minimum -Maximum -StandardDeviation

# Command run twice and second summary taken
Count             : 10
Average           : 410.43762
Sum               :
Maximum           : 479.3415
Minimum           : 378.843
StandardDeviation : 27.3532692743502
Property          :
# Powershell 7.4.0-preview.3-81-g6b80bf03b74dff76816740fbb3549b87f190f518 - Adding OperationTimeoutSeconds parameter
1 .. 10 | %{(Measure-Command { iwr http://localhost:5000/ -OperationTimeoutSeconds 10 -OutFile iwr_7.4.0-preview.3-with-19558-OperationTimeoutSeconds-data.bin }).TotalMilliseconds} | Measure-Object -Average -Minimum -Maximum -StandardDeviation

# Command run twice and second summary taken
Count             : 10
Average           : 646.04633
Sum               :
Maximum           : 663.085
Minimum           : 607.9275
StandardDeviation : 16.2551572417324
Property          :

My conclusions from these results are:

• the patch does not have any material impact on performance on downloading a file from the last preview release when the -OperationTimeoutSeconds parameter isn't used
• the patch has a 30% increase on download performance over release 3 when the -OperationTimeoutSeconds parameter is used but is still favourable when compared to curl performance. This is likely attributable to the work spent resetting the timeout on the cancellation token before each network read.
• the last preview release is significantly faster than 7.3.4 for this use case as it is now faster than using curl when it was significantly slower in the 7.3.4 release.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
      (which runs in a different PS Host).
      • Issue filed:
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Issue filed:
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Issue filed:
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
      • Issue filed:

[stevenebutler]

Reopening - accidentally force-pushed empty commits after a squash merge.

[TravisEz13]

LGTM as far as the code.

@PowerShell/powershell-maintainers Is this a breaking change because public interfaces to a cmdlet was changed?

[TravisEz13]

The changes seem significant with regard to memory handling. We should consider running performance testing before and after the change. Especially for downloading large files (at least 200MiB). This scenario is already slow, any performance regression may make the feature unusable.

I don't see anything particularly bad, but we should test for the unexpected.

[iSazonov]

1. I think this PR is wrong. There's a fundamental problem there HttpClient Response Stream hangs dotnet/runtime#74568 (comment)
   It should be solved in .Net, rather than creating a lot of useless code in PowerShell.

2. That issue also clearly explains why the NetworkTimeout parameter makes no sense at all.

3. Such an unnecessary complication Invoke-RestMethod/WebRequest: Support CTRL-C when reading data and connection hangs #19330 This should be rolled back. The same thing can be done in a much simpler way. Not only is the code unnecessarily complicated, but new bugs seem to have been created.

I urge MSFT team to look at this carefully.

[SteveL-MSFT]

@PowerShell/wg-powershell-cmdlets reviewed this, we agree with the added functionality. However, we suggest renaming TimeoutSec to ConnectionTimeoutSeconds and the new parameter OperationTimeoutSeconds as "NetworkTimeoutSec" can still be a bit ambiguous what it is referring to. We also prefer to spell out "Seconds" since the parameter name is already verbose and could be confused with "Security".

[stevenebutler]

Setting to WIP while I address WG recommendations.

[stevenebutler]

Ok, I have finished updating as per recommendations. I also did some minor refactoring of the timeout calculation code to share the conversion from integer seconds to a TimeSpan between the two timeouts.

[SteveL-MSFT]

Great change! Overall, looks good, but one comment on the fully qualified error id.

[pull-request-quantifier-deprecated]

This PR has 303 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       : +226 -77
Percentile : 70.3%

Total files changed: 9

Change summary by file extension:
.cs : +152 -67
.ps1 : +74 -10

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.

[pull-request-quantifier-deprecated]

This PR has 303 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       : +226 -77
Percentile : 70.3%

Total files changed: 9

Change summary by file extension:
.cs : +152 -67
.ps1 : +74 -10

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.

[daxian-dbw]

@stevenebutler Thanks for your contribution!

[ghost]

🎉v7.4.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes