-
Notifications
You must be signed in to change notification settings - Fork 353
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
RemoteExecutor Swallows Errors and Throws Invalid Exceptions #6371
Comments
Ok... So I created a separate unit test project to confirm this. [Fact]
public void Test1()
{
Assert.True(RemoteExecutor.IsSupported);
RemoteExecutor.Invoke(() =>
{
Assert.True(false);
}).Dispose();
RemoteExecutor.Invoke(() =>
{
return RemoteExecutor.SuccessExitCode;
}, new RemoteInvokeOptions { CheckExitCode = true }).Dispose();
} |
Confirmed to not be isolated to my machine. Linux Pass https://github.com/JimBobSquarePants/RemoteExecutorTests/runs/1232480114 |
• CoreHostLibMissingFailure (-2147450749 0x80008083) - One of the dependent libraries is missing. Typically when the hostfxr, hostpolicy or coreclr dynamic libraries are not present in the expected locations. Probably means corrupted or incomplete installation. |
https://github.com/dotnet/runtime/search?q=CoreHostLibMissingFailure Enabling host tracing might help? COREHOST_TRACE=1 COREHOST_TRACE_VERBOSITY=4 |
Thanks @danmosemsft . I thought it was something like that and have been attempting to determine whether there is a specific SDKs missing that should be there. It's very odd that the GitHub Ubuntu image works but Windows doesn't. Is |
It's an environment variable read by the host - hostfxr.dll, I think (this isn't my area). So you should be able to set it anywhere eg before launching dotnet test, or in your test just before launching the remote executor. |
Thanks. I’ll have a crack and report back |
No further info I could uncover with that variable. Just lots of build noise. |
Does the logging not end with an error? Hmm. @vitek-karas I believe owns the host and can help advise from here. |
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:05.66] ExecutorTest.UnitTest1.ShouldNotThrow [FAIL]
[xUnit.net 00:00:05.68] ExecutorTest.UnitTest1.ShouldThrow [FAIL]
X ExecutorTest.UnitTest1.ShouldNotThrow [295ms]
Error Message:
Exit code was -2147450749 but it should have been 42
Expected: True
Actual: False
Stack Trace:
at Microsoft.DotNet.RemoteExecutor.RemoteInvokeHandle.Dispose(Boolean disposing) in /_/src/Microsoft.DotNet.RemoteExecutor/src/RemoteInvokeHandle.cs:line 237
at ExecutorTest.UnitTest1.ShouldNotThrow() in D:\a\RemoteExecutorTests\RemoteExecutorTests\ExecutorTest\UnitTest1.cs:line 23
X ExecutorTest.UnitTest1.ShouldThrow [28ms]
Error Message:
Assert.Throws() Failure
Expected: typeof(Microsoft.DotNet.RemoteExecutor.RemoteExecutionException)
Actual: (No exception was thrown)
Stack Trace:
at ExecutorTest.UnitTest1.ShouldThrow() in D:\a\RemoteExecutorTests\RemoteExecutorTests\ExecutorTest\UnitTest1.cs:line 13
Test Run Failed.
Total tests: 2
Failed: 2
Total time: 14.8005 Seconds
Execute managed assembly exit code: 0x1
Waiting for breadcrumb thread to exit...
Done waiting for breadcrumb thread to exit...
C:\Program Files\dotnet\sdk\3.1.402\Microsoft.TestPlatform.targets(32,5): error MSB4181: The "Microsoft.TestPlatform.Build.Tasks.VSTestTask" task returned false but did not log an error. [D:\a\RemoteExecutorTests\RemoteExecutorTests\ExecutorTest\ExecutorTest.csproj]
Execute managed assembly exit code: 0x1
Waiting for breadcrumb thread to exit...
Done waiting for breadcrumb thread to exit...
Execute managed assembly exit code: 0x1
Waiting for breadcrumb thread to exit...
Done waiting for breadcrumb thread to exit...
Execute managed assembly exit code: 0x1
Waiting for breadcrumb thread to exit...
Done waiting for breadcrumb thread to exit...
Error: Process completed with exit code 1.
3s
|
Ah, I don't see the host logging there. Either it's not enabled, or not captured. Does host-tracing.md above help? |
There's definitely a lot of extra logging 100s of extra lines, just nothing relevant (that I could see). Here's a previous run. I'll try moving to Core 3.1 and adding the extra verbosity command |
The host trace only got all of the "dotnet test" stuff (starts 4 processes internally) + the powershell (which is I guess what runs the whole thing). I assume this doesn't have a local repro... right? |
As far as I can tell the remove executor has no way to capture stdout or stderr of the child process it spawns - so normal
That should give us a way to get the trace. (And would disable the env. variables set for the entire run, as that just creates noise which is not useful). |
Thanks @vitek-karas Here's the result of running the test locally with the following code (I'm assuming that's what you meant?). I haven't checked the output via CI yet. (I've had two people independently check and confirm on their Windows machines in addition to my local device and CI.) [Fact]
public void ShouldThrow()
{
var tracePath = Path.Combine(Path.GetTempPath(), $"remoteexecutor{DateTime.Now:yyyyMMdd}.txt");
Assert.True(RemoteExecutor.IsSupported);
var psi = new ProcessStartInfo();
psi.Environment[$"COREHOST_TRACE"] = "1";
psi.Environment[$"COREHOST_TRACEFILE"] = tracePath;
Assert.Throws<RemoteExecutionException>(() =>
RemoteExecutor.Invoke(() => Assert.True(false),
new RemoteInvokeOptions
{
StartInfo = psi
}).Dispose()
);
} Output is the same in Debug and Release modes. (Differs only in bin subpath)
|
Thanks - I was able to repro it locally. The command line it executes doesn't seem to make sense to me: F:\AppModel\repro\RemoteExecutorTests\ExecutorTest\bin\Debug\netcoreapp3.1\testhost.exe
exec
--runtimeconfig "F:\AppModel\repro\RemoteExecutorTests\ExecutorTest\bin\Debug\netcoreapp3.1\ExecutorTest.runtimeconfig.json"
--depsfile "F:\AppModel\repro\RemoteExecutorTests\ExecutorTest\bin\Debug\netcoreapp3.1\ExecutorTest.deps.json"
"F:\AppModel\repro\RemoteExecutorTests\ExecutorTest\bin\Debug\netcoreapp3.1\Microsoft.DotNet.RemoteExecutor.dll"
<args to the remote executor - not interesting> This would work if it were running "dotnet.exe exec ...", but I doubt testhost recognizes "exec" (I could be wrong though). In any case the command line won't work because the special host parameters must be first on the command line: I tried running the same command line via I don't know how this passes tests (if they run) in Arcade repo and/or how it's used in a case where it would actually work. But as is, this seems completely broken. Also - I which we could update the I think somebody from Arcade should comment on what is the intended command line. I can definitely help with figuring out what it should be in reality to get all the desired behaviors. Couple of notes about what I think it SHOULD be doing:
The break was probably introduced by changes to the VSTest platform - some time ago it switched from running the test via "dotnet.exe" to using the "testhost.exe". But that's been quite a while now if I remember correctly. |
@ViktorHofer might have context |
Thanks for the help so far. Thought I was going mad! |
Thanks for reporting @JimBobSquarePants and @vitek-karas for the analysis. Vitek is right that RemoteExecutor doesn't set As a simple fallback mechanism we could check if processFileName ends with (dotnet[.exe]) and if not, use the dotnet in the %PATH%. For a more complete implementation, we should follow what Vitek proposes:
An implementation of that could look like this:
@vitek-karas do you think we should stop at some point climbing up the directory tree? For self-contained application we need to fallback to %PATH% as a self-contained application doesn't have a dotnet host. Right? |
Regarding testing: I was wondering why the RemoteExecuter tests are passing: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.RemoteExecutor/tests/RemoteExecutorTests.cs As for the fix: If the parent test was executed via The problem is all the cases where the above will not work:
Crazy idea: Use startup hook. This would in theory work everywhere where startup hook is correctly supported which is currently basically everywhere except for trimmed apps and AOT. https://github.com/dotnet/runtime/blob/master/docs/design/features/host-startup-hook.md
Startup hook would allow the remote executor to run its own code before /cc @agocke - who is looking into running tests as single-file |
Is there anything useful I can do to expediate a fix here? |
@JimBobSquarePants We need to agree on the approach. If you want to spend some time on this, I think prototyping one of the proposed solutions above would be a good start - to see if it really works and how hard/complex of a solution it is. Ideally we would pick a solution which we can extend going forward to as many scenarios as possible, but that can be long term. Short term we could go with a simple fix for whatever problem we're hitting currently. The simplest right now if probable fixing the code to find the right "dotnet.exe" (if available) and use that. That should fix the scenario described in this issue. |
I very much doubt I have the domain knowledge required to implement a fix tbh. Was thinking more test scenarios. |
We're hitting the same issue in dotnet/winforms... And this is kind of blocking us too. There is nothing in our configs (test or otherwise) that sets "self contained" flag, so "Failed to run as a self-contained app" is kind of puzzling.
|
I think this nicely describes what happened and why it was trying to run the app as self-contained. As for the solutions mentioned above - I actually like the startup hook idea even more now. It would get us the exact same environment as the original test, and I don't feel like this is too hacky anymore. |
I just started to work on a minimalistic fix before @RussKie commented. There are more and more PR-s in ImageSharp with out-of-process tests, and our coverage became very unreliable. We need a fix ASAP. I don't have the time and the knowledge for a full-scale solution using startup hooks. I believe the most urgent thing is to get this fixed for the common Windows cases as simply and as quickly as possible. (Climb up to find Alternative idea: Remove |
I don't have any problem with a partial fix. I'm curious: In your comment you mention that it makes your coverage unreliable. Does it mean that it fails only sometimes, or that you have to avoid running certain configurations entirely because of this? |
We keep running all configurations (hoping in an eventual fix), it's just that the Windows ones are always passing, regardless of the actual outcome of the body function. IMO it's matter of time until it leads to false positives because of a platform-specific issue, and I'm afraid we will be rolling out buggy packages to NuGet because of this, thus the use of the term "unreliable coverage". (@JimBobSquarePants can correct me if there's something wrong in my thinking.) |
Yep. That's what is happening. We were passing buggy code before we noticed. It's a growing elephant as we introduce more and more performance features. |
Haha, if only I read the test log :)
This approach shifts the onus of bootstrapping to the consumer of the RE. Whilst this may fair to suggest this direction for custom scenarios you'll need to convince me why running a test with the RE from under VS is not a mainstream scenario. |
My understanding is that this is Arcade-only functionality, so the experience is not polished like a shipping product. At the same time, I don't really understand what this runner is supposed to do. Try to replicate the currently loaded assembly as best as possible in a new process, then call it? @vitek-karas's right that this won't work for Native AOT-like scenarios, but I think that's because this idea is built on some basic principles:
These all seem like pretty shaky principles to me, when looked at from a variety of configurations. I'd want to look closer at all of them before I shipped a solution like this. |
I'm afraid it is hard to agree with this statement - https://github.com/dotnet/runtime/search?q=RemoteExecutor yields 187 results. The RE is also explained in the .NET runtime docs. In winforms we have started using it in late 2019, and at no point it was called out as "arcade specific, don't use".
IMO the issue is not about this. The issue is that we found an issue that doesn't appear to work in VS (the main scenario), and about fixing it. |
This tool originated in dotnet/runtime before it was generalized here. It's used throughout the unit tests to isolate tests that affect process wide state, or need to have special process state, and also in some special cases eg a test for fail fast. |
Right, the remote executor is not polished like a shipping product. It is built on shaky principles, but that is ok since it is only meant to be good enough for limited .NET team own needs, like everything else in the Arcade repo. As of right now, the remote executor does not work and it is automatically disabled for single file deployment (NativeAOT is just one particular type of single file deployment) and it is also disabled on browser and mobile OSes. See |
Given the current project status, the patch that Vitek suggested to work-around issues in .NET core projects seems reasonable. In short, I wouldn't put in much more work than necessary to achieve the immediate needs. |
Which one? |
Looking around for a |
Ah, you mean #7426 :) |
I don't understand this to be honest. The proposal using startup hooks would be that RE implements its "client" as a startup hook assembly (as oppose to entry point assembly the way it's now). When it runs the client process it would setup the environment to load the client component as a startup hook (it already sets up other environment so this is not a big change). The startup hook would then perform the actual test run and exit the process. The reason why this approach is cleaner is that startup hooks are a fully supported feature to inject code into (almost) any application - lot less need for hacking things to load the test code into a new app. There should be no visible change to the user of RE, it would just change the implementation of RE. As for the rest of the comments - this is absolutely not a shipping product. And it's expected that certain things won't work, since we didn't need them to work yet. |
I guess I misinterpreted your comment, as it wasn't particular obvious if the hook should be a part of the Arcade/RemoteExecutor, or something a consumer should do. |
100% agreed. Even though it's an open source project, there is no support policy attached to it. When I ported this library over from CoreFx couple years ago, I only did so to share the implementation with ML.NET, aspnetcore and later winforms. We try our best to resolve reported issues but RemoteExecutor was never intended to support every possible variant like single file applications, mobile, etc. As an example I removed the UWP support as well as we stopped testing it in CoreFx's main branch. |
I could be holding it wrong but it does not seem to be working as expected.
Microsoft.DotNet.RemoteExecutor Version 6.0.0-beta.20508.3 (Are there non beta version available?)
Installed via https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json
The following method runs without any assertation exceptions. I would expect it to fail.
The following code throws an exception. I would expect none.
Message:
Exit code was -2147450749 but it should have been 42
Expected: True
Actual: False
Stack Trace:
RemoteInvokeHandle.Dispose(Boolean disposing) line 237
RemoteInvokeHandle.Dispose() line 58
The text was updated successfully, but these errors were encountered: