Skip to content

Address feedback from #78648#78880

Merged
RikkiGibson merged 4 commits intodotnet:mainfrom
RikkiGibson:fbp-address-prev-feedback
Jun 20, 2025
Merged

Address feedback from #78648#78880
RikkiGibson merged 4 commits intodotnet:mainfrom
RikkiGibson:fbp-address-prev-feedback

Conversation

@RikkiGibson
Copy link
Member

Follow up to #78648

@RikkiGibson RikkiGibson marked this pull request as ready for review June 11, 2025 02:31
@RikkiGibson RikkiGibson requested a review from a team as a code owner June 11, 2025 02:31
CreateNoWindow = true,
UseShellExecute = false,
RedirectStandardInput = redirectStandardInput,
RedirectStandardInput = true,
Copy link
Member

Choose a reason for hiding this comment

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

We probably then need to have an explicit close of the standard input stream; otherwise something that were to try to read is now going to wait (forever) for that to complete.

Copy link
Member Author

Choose a reason for hiding this comment

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

The component which would block, is the child dotnet process which is calling Console.ReadLine() or similar, right?

Are you asking to bring back a flag like bool leaveStandardInputOpen or some such, so that this component can close it if needed?

Copy link
Member

Choose a reason for hiding this comment

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

The component which would block, is the child dotnet process which is calling Console.ReadLine() or similar, right?

Yep.

If we don't say to redirect, we'll be sharing the stdin from the parent process...which if the user is lucky, reading from it will fail. Note emphasis on lucky, because the alterative is an undiagnosable hang. If we redirect and don't close it, we'll definitely hang. So without the other line this might make things worse.

And of course @dibarbet might have other thoughts here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants