Skip to content
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

Executor: Don't use Process.ExitCode, unless the process has exited #1947

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

radical
Copy link
Member

@radical radical commented Mar 17, 2022

Once the AfterAll signal is received, and the process doesn't exit within 2s, we try to kill the process tree with SIGTERM. But the process might not immediately quit, so trying to access process.ExitCode with the process still running throws an exception:

[2022/03/17 05:48:51][INFO] // AfterAll
[2022/03/17 05:48:53][INFO] // The benchmarking process did not quit on time, it's going to get force killed now.
[2022/03/17 05:48:53][INFO] Unhandled exception. System.InvalidOperationException: Process must exit before requested information can be determined.
[2022/03/17 05:48:53][INFO]    at System.Diagnostics.Process.EnsureState(State state)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Toolchains.Executor.Execute(Process process, BenchmarkCase benchmarkCase, SynchronousProcessOutputLoggerWithDiagnoser loggerWithDiagnoser, ILogger logger, ConsoleExitHandler consoleExitHandler, Int32 launchIndex)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Toolchains.Executor.Execute(BenchmarkCase benchmarkCase, BenchmarkId benchmarkId, ILogger logger, ArtifactsPaths artifactsPaths, String args, IDiagnoser diagnoser, IResolver resolver, Int32 launchIndex)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Toolchains.Executor.Execute(ExecuteParameters executeParameters)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Running.BenchmarkRunnerClean.RunExecute(ILogger logger, BenchmarkCase benchmarkCase, BenchmarkId benchmarkId, IToolchain toolchain, BuildResult buildResult, IResolver resolver, IDiagnoser diagnoser, Int32 launchIndex)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Running.BenchmarkRunnerClean.Execute(ILogger logger, BenchmarkCase benchmarkCase, BenchmarkId benchmarkId, IToolchain toolchain, BuildResult buildResult, IResolver resolver)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Running.BenchmarkRunnerClean.RunCore(BenchmarkCase benchmarkCase, BenchmarkId benchmarkId, ILogger logger, IResolver resolver, BuildResult buildResult)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Running.BenchmarkRunnerClean.Run(BenchmarkRunInfo benchmarkRunInfo, Dictionary`2 buildResults, IResolver resolver, ILogger logger, List`1 artifactsToCleanup, String resultsFolderPath, String logFilePath, Int32 totalBenchmarkCount, StartedClock& runsChronometer, Int32& benchmarksToRunCount)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Running.BenchmarkRunnerClean.Run(BenchmarkRunInfo[] benchmarkRunInfos)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Running.BenchmarkSwitcher.RunWithDirtyAssemblyResolveHelper(String[] args, IConfig config, Boolean askUserForInput)
[2022/03/17 05:48:53][INFO]    at BenchmarkDotNet.Running.BenchmarkSwitcher.Run(String[] args, IConfig config)
[2022/03/17 05:48:53][INFO]    at MicroBenchmarks.Program.Main(String[] args) in /home/helixbot/work/A60608D9/p/performance/src/benchmarks/micro/Program.cs:line 42

radical added 2 commits March 17, 2022 15:17
When the first build fails, we try to run it again with
`--no-dependencies`, but don't show the build output from the first
build. This can hide build errors.
@radical
Copy link
Member Author

radical commented Mar 17, 2022

cc @adamsitnik @lewing

@radical
Copy link
Member Author

radical commented Mar 17, 2022

For a future PR - the exception should have been caught somewhere, and not crashed the whole thing. It could maybe be caught in BenchmarkRunnerClean:Run, or RunCore, but I wasn't sure what should be done in that case.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Thank you for the fix @radical!

@adamsitnik adamsitnik merged commit 01ead56 into dotnet:master Mar 18, 2022
@radical radical deleted the fix-process-exitcode-bug branch March 18, 2022 15:12
@AndreyAkinshin AndreyAkinshin added this to the v0.13.2 milestone Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants