-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Handle exception thrown by ProcessName during ProcessEx timeout #23990
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.
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
Conversation
Co-authored-by: Brennan <[email protected]>
4cb297d
to
c0a1346
Compare
Hello @ajaybhargavb! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
// We greedily create a timeout exception message even though a timeout is unlikely to happen for two reasons: | ||
// 1. To make it less likely for Process getters to throw exceptions like "System.InvalidOperationException: Process has exited, ..." | ||
// 2. To ensure if/when exceptions are thrown from Process getters, these exceptions can easily be observed. | ||
var timeoutExMessage = $"Process proc {proc.ProcessName} {proc.StartInfo.Arguments} timed out after {DefaultProcessTimeout}."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late questions (sorry):
- Have you confirmed
ProcessName
is available here i.e. the process is always started before the constructor is called❔ - Do we need a similar change in src/Tools/Microsoft.dotnet-openapi/test/ProcessEx.cs and wherever else we have similar
Process
wrappers❔ (No, I don't remember why we have more than oneProcessEx
class.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_process
is private and there's no mechanism in ProcessEx to start a process, so it must be started here.- That's probably a good idea. I didn't know there were copies of this. Is that the only other one?
Process.ProcessName throwing in your CT callback caused BlazorTemplates.Tests to hang on our CI server.
https://dev.azure.com/dnceng/public/_build/results?buildId=731663&view=results