Leave the input/output/error handles unset when they are not redirected by daxian-dbw · Pull Request #20853 · PowerShell/PowerShell

daxian-dbw · 2023-12-05T21:55:43Z

PR Summary

This PR is to fix Start-Process to address PowerShell/PSReadLine#288.

Basically, when starting pwsh with Start-Process in the following ways, the new console and the existing console get weirdly tied together, which causes Console.ReadKey running in those 2 consoles to interfere with each other (block and unblock each other):

Start-Process pwsh -RedirectStandardError .\error.txt: start pwsh with its stderr redirected to the file error.txt
Start-Process pwsh -PassThru -Credential (Get-Credential <another-user>): start pwsh as a different user.

Based on the investigation in PowerShell/PSReadLine#288 (comment), we should not explicitly set the standard input/output/error handles when they are not redirected. Instead, just let Windows figure out the default to use when creating the process.

Note that, the change in TaskbarJumpList.cs is related. When running Start-Process pwsh -Credential as a non-admin user, the "TaskbarJumpList" code throws AccessDenied exception, which causes a debug build to crash. The Debug.Fail is not needed. The catch (ThreadStartException) below simply ignore the exception too.

Manual validation

I verified the changes on both Windows 11 and Windows 10, and it works fine for the abovementioned scenarios. I also verified that the "redirecting input/output/error to files" are still working fine, regardless of using -Credential or not.

PR Checklist

daxian-dbw · 2023-12-05T21:56:30Z

src/Microsoft.PowerShell.ConsoleHost/WindowsTaskbarJumpList/TaskbarJumpList.cs

                    var CLSID_EnumerableObjectCollection = new Guid(@"2d3468c1-36a7-43b6-ac24-d3f02fd9607a");
                    const uint CLSCTX_INPROC_HANDLER = 2;
                    const uint CLSCTX_INPROC = CLSCTX_INPROC_SERVER | CLSCTX_INPROC_HANDLER;
-                    var ComSvrInterface_GUID = new Guid(@"555E2D2B-EE00-47AA-AB2B-39F953F6B339");


This variable is assigned but not used anywhere.

pull-request-quantifier-deprecated · 2023-12-05T22:00:48Z

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

Quantification details

Label      : Extra Small
Size       : +9 -14
Percentile : 9.2%

Total files changed: 2

Change summary by file extension:
.cs : +9 -14

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.

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

src/Microsoft.PowerShell.ConsoleHost/WindowsTaskbarJumpList/TaskbarJumpList.cs

iSazonov

What if user redirect only output?

Can we cover the scenario we fix by tests?

daxian-dbw · 2023-12-06T18:46:30Z

What if user redirect only output?

When starting pwsh with only the output redirected, PSReadLine will not be loaded because it doesn't work when output is redirected. So, this scenario is not in our consideration for the bug.

Can we cover the scenario we fix by tests?

Existing tests for Start-Process still work. The particular issue that was fixed was a pure interactive scenario, so I don't think we can cover it by test.

…ed (PowerShell#20853)

ltrzesniewski · 2025-02-10T21:00:03Z

I'm pretty sure this change broke atuinsh/atuin#2543 - looks like letting Windows figure out the default handle to use doesn't always work as well as expected. 😢

In that PR, I want to redirect stderr to a file while keeping stdout on the screen. After updating PowerShell to v7.5.0, nothing appears on screen anymore on Windows.

The following line now misbehaves:

Start-Process -Wait -NoNewWindow -RedirectStandardError $resultFile.FullName -FilePath atuin -ArgumentList $argString

If I understand correctly, there's no easy workaround for this. You can't supply a custom output handle to ProcessStartInfo, you have to implement all the redirection yourself, which is not something I'm particularly happy to do in a PowerShell script. If you know a better solution, I'd be happy to hear.

iSazonov · 2025-02-11T03:09:53Z

@ltrzesniewski Please open new issue with simple repro steps.

ltrzesniewski · 2025-02-11T10:32:53Z

Sure, sorry. Reported here: #24986

Leave the intput/output/error handles unset when they are not redirected

5d41a18

daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 5, 2023

daxian-dbw requested review from SeeminglyScience and SteveL-MSFT December 5, 2023 21:55

daxian-dbw self-assigned this Dec 5, 2023

daxian-dbw requested review from adityapatwardhan and anmenaga as code owners December 5, 2023 21:55

pull-request-quantifier-deprecated bot added the Extra Small label Dec 5, 2023

daxian-dbw commented Dec 5, 2023

View reviewed changes

Some cleanup

eac6c3b

daxian-dbw mentioned this pull request Dec 5, 2023

Bug when launching a new Powershell instance with Start-Process and alternate credentials PowerShell/PSReadLine#288

Closed

iSazonov reviewed Dec 6, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs Show resolved Hide resolved

iSazonov reviewed Dec 6, 2023

View reviewed changes

src/Microsoft.PowerShell.ConsoleHost/WindowsTaskbarJumpList/TaskbarJumpList.cs Show resolved Hide resolved

iSazonov reviewed Dec 6, 2023

View reviewed changes

iSazonov approved these changes Dec 8, 2023

View reviewed changes

daxian-dbw merged commit 6a90fd0 into PowerShell:master Dec 8, 2023

daxian-dbw deleted the start-process branch December 8, 2023 20:20

mklement0 mentioned this pull request Jan 1, 2024

Start-Process -Credential prevents interactive input in both the caller and the new window #12129

Closed

chrisdent-de pushed a commit to chrisdent-de/PowerShell that referenced this pull request Sep 12, 2024

Leave the input/output/error handles unset when they are not redirect…

d817504

…ed (PowerShell#20853)

ltrzesniewski mentioned this pull request Feb 10, 2025

Add PowerShell module atuinsh/atuin#2543

Merged

2 tasks

ltrzesniewski mentioned this pull request Feb 11, 2025

Start-Process in 7.5.0: invalid handles, no output emitted on Windows #24986

Closed

5 tasks

daxian-dbw mentioned this pull request Feb 20, 2025

Set standard handles explicitly when starting a process with -NoNewWindow #25061

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leave the input/output/error handles unset when they are not redirected#20853

Leave the input/output/error handles unset when they are not redirected#20853
daxian-dbw merged 2 commits intoPowerShell:masterfrom
daxian-dbw:start-process

daxian-dbw commented Dec 5, 2023 •

edited

Loading

Uh oh!

daxian-dbw Dec 5, 2023

Uh oh!

pull-request-quantifier-deprecated bot commented Dec 5, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

Uh oh!

Uh oh!

iSazonov left a comment

Uh oh!

daxian-dbw commented Dec 6, 2023

Uh oh!

ltrzesniewski commented Feb 10, 2025

Uh oh!

iSazonov commented Feb 11, 2025

Uh oh!

ltrzesniewski commented Feb 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

daxian-dbw commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Manual validation

PR Checklist

Uh oh!

daxian-dbw Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

pull-request-quantifier-deprecated bot commented Dec 5, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

Uh oh!

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Dec 6, 2023

Uh oh!

ltrzesniewski commented Feb 10, 2025

Uh oh!

iSazonov commented Feb 11, 2025

Uh oh!

ltrzesniewski commented Feb 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daxian-dbw commented Dec 5, 2023 •

edited

Loading