Invoke-RestMethod/WebRequest: Support CTRL-C when reading data using cancellation token

[stevenebutler]

APIs that should be cancelled are passed the cancellation token and _cancelToken is configured with CancelAfter() when request timeout is specified.

Switched use of synchronous stream APIs to asynchronous APIs that use the cancellation token.

PR Summary

This PR closes #16145

The root cause of these issues was that once the response headers are read, the cancellation token was not always being passed to APIs that deal with reading the response stream.

In some scenarios, CTRL-C worked (where the cancellation token was being passed down), but this was not occurring in all scenarios and the use of blocking read APIs that do not take cancellation tokens prevented the early cancellation when CTRL-C was used.

PR Context

This PR helps because it allows all I/O in the web requests cmdlets to be cancelled if the user uses CTRL-C to terminate processing.

A previous iteration of this PR added a timeout, but was incorrect as the desired behaviour is to cancel if the connection is stalled for 5 minutes, not after a fixed timeout.

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]

@CarloToso and @iSazonov - could you please provide some feedback on this approach. I have tested the scenarios in the linked bugs, as well as downloading with a stalling RSS feed using a custom minimal API test server and can confirm the CTRL-C and timeout works for me as expected with these changes.

Also, modifying some of these files has given me a large number of codefactor issues that already existed in those files. Do these need to be fixed as part of this PR?

[CarloToso]

@stevenebutler Fixing the codefactor issues would be good, you could also add some tests.

[CarloToso]

This use of TimeOut sec is not correct, it should indicate the timeout before the connetion to the server, not during the download
test:

iwr https://software-static.download.prss.microsoft.com/dbazure/988969d5-f34g-4e03-ac9d-1f9786c66751/22621.525.220925-0207.ni_release_svc_refresh_CLIENTENTERPRISEEVAL_OEMRET_x64FRE_en-us.iso -Timeout 5
#--> fails after 5 seconds

It should only fail if Invoke-WebRequest cant connect to the server after trying for 5 seconds

[stevenebutler]

Yes, I did ask about this in #12249 comments. I thought this needed some discussion but wanted to get a working prototype out for discussion.

Should we use a second parameter like ReceiveTimeout with default of 5 minutes or do something else?

[CarloToso]

I think the desired behaviour is checking if the download is stuck for 5 minutes and then closing the download with a warning message. At the moment your implementation halts the successful download after a timespan (-ReceiveTimeout) has passed. This could be a new feature (you should open an issue to discuss if we should implement it), but it's not the correct solution for issue #12249.
This PR at the moment fixes #16145 (Ctrl-c on download stuck because of no internet)

[stevenebutler]

Ok, I see. I think implementing what you describe is likely to be a more complex patch as it would need to monitor the stream progress and adjust cancellation timeouts whenever data is received. Given this PR still address #16145 I think it would be good to submit (with the timeout removed) so that issue can be closed, if it is agreed that this is the correct way to deal with that issue.

[CarloToso]

We will have to wait for @iSazonov 's rewiew, but it looks good to me 👍

[iSazonov]

My comment #12249 (comment)

[stevenebutler]

@iSazonov I have responded to your comment in the issue. I don't think using a separate task is the right solution as I have explained there. Could you please review this PR as it demonstrates that using a task is not necessary to support cancellation. Provided you are okay with the PR I will go back and fix the codefactor issues in the code base that are being flagged.

I will try to build timeout cancellation on top of this PR once it is approved using the approach I outlined in #12249.

[stevenebutler]

FYI, I have fixed reported codefactor issues and merged recent changes from master branch.

[stevenebutler]

@stevenebutler Fixing the codefactor issues would be good, you could also add some tests.

I’m not sure how I could write tests for this. Do you have any suggestions?

my testing so far was mostly ctrlc and time-out testing with pulling network cables. Is it possible to simulate ctrlc in a pester test? A timeout is possible to simulate if the test web server supports that but I haven’t looked at it’s capabilities yet. I wrote a simple minimal api test server that verified the cancellation token worked for the rss reader logic as well by writing out chunks of rss with delays.

Should something like that get integrated into the test server (if it can’t already do this sort of thing)?

[CarloToso]

@stevenebutler don't worry about the tests, I don't we can simulate accurately the issue this PR fixes

[pull-request-quantifier-deprecated]

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

---

Quantification details

Label      : Medium
Size       : +62 -49
Percentile : 42.2%

Total files changed: 7

Change summary by file extension:
.cs : +62 -49

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.