Skip to content

Make all RemoteExecutor.Invoke overloads check exit code by default #8369

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

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

elinor-fung
Copy link
Member

Currently, only the overloads where the invoked method returns an int or Task<int> check for an expected exit code. This meant that crashes (not caught as a managed exception) would not be detected when those overloads were used. This changes all the overloads to check the exit code by default.

Resolves #5865

cc @stephentoub

Copy link
Member

@stephentoub stephentoub 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. Are you able to validate testing runtime with these changes? I'm wondering if there are any problems silently lurking.

@elinor-fung
Copy link
Member Author

Kicked off a runtime build using the updated RemoteExecutor from this change (assuming I set it up correctly this time): https://dnceng.visualstudio.com/public/_build/results?buildId=1565764

@elinor-fung
Copy link
Member Author

A couple tests were assuming the exit code would not be validated - opened dotnet/runtime#64133.

There is also a consistent crash in System.Tests.TimeZoneInfoTests.NJulianRuleTest that we have been missing - filed dotnet/runtime#64134.

@elinor-fung elinor-fung merged commit 07a25f4 into dotnet:main Jan 24, 2022
@elinor-fung elinor-fung deleted the remoteExecutorExitCode branch January 24, 2022 19:17
@danmoseley
Copy link
Member

Thanks for fixing this @elinor-fung .

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.

RemoteExec ignores AV's and CLR crashes
3 participants