8000 Invoke-RestMethod/WebRequest: Support CTRL-C when reading data using cancellation token by stevenebutler · Pull Request #19315 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

stevenebutler
Copy link
Contributor
@stevenebutler stevenebutler commented Mar 12, 2023 •
edited
Loading

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

@ghost ghost assigned daxian-dbw Mar 12, 2023
@stevenebutler stevenebutler changed the title Invoke-RestMethod/WebRequest: Support read timeout using cancellation token WIP: Invoke-RestMethod/WebRequest: Support read timeout using cancellation token Mar 12, 2023
@stevenebutler
Copy link
Contributor Author

@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
Copy link
Contributor
CarloToso commented Mar 12, 2023 •
edited
Loading

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

CarloToso
CarloToso reviewed Mar 12, 2023
View reviewed changes
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated
@@ -563,12 +563,18 @@ protected override void ProcessRecord()

8000 using HttpResponseMessage response = GetResponse(client, request, handleRedirect);

if (TimeoutSec > 0)
Copy link
Contributor
@CarloToso CarloToso Mar 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment #12249 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@stevenebutler
Copy link
Contributor Author
stevenebutler commented Mar 12, 2023 •
edited
Loading

@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)?

8000 stevenebutler changed the title WIP: Invoke-RestMethod/WebRequest: Support read timeout using cancellation token WIP: Invoke-RestMethod/WebRequest: Support CTRL-C when reading data using cancellation token Mar 12, 2023
@CarloToso
Copy link
Contributor

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

@daxian-dbw daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 13, 2023
@stevenebutler stevenebutler mentioned this pull request Mar 14, 2023
Closed
@stevenebutler stevenebutler force-pushed the fix-invoke-restmethod-will-not-timeout-when-connection-lost branch from 6bb2521 to bae7acd Compare March 15, 2023 09:38
@stevenebutler stevenebutler force-pushed the fix-invoke-restmethod-will-not-timeout-when-connection-lost branch from bae7acd to 835b508 Compare March 15, 2023 09:46
@stevenebutler stevenebutler changed the title WIP: Invoke-RestMethod/WebRequest: Support CTRL-C when reading data using cancellation token Invoke-RestMethod/WebRequest: Support CTRL-C when reading data using cancellation token Mar 15, 2023
@stevenebutler stevenebutler deleted the fix-invoke-restmethod-will-not-timeout-when-connection-lost branch March 15, 2023 09:52
@pull-request-quantifier-deprecated
Copy link

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.

@stevenebutler stevenebutler mentioned this pull request Mar 15, 2023
Merged
22 tasks
@CarloToso CarloToso mentioned this pull request Mar 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iSazonov iSazonov iSazonov left review comments

@CarloToso CarloToso CarloToso left review comments

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

Assignees

@daxian-dbw daxian-dbw

Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Medium
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Invoke-WebRequest not responding to «Ctrc+c»
4 participants
@stevenebutler @CarloToso @iSazonov @daxian-dbw
0