bilal-fazlani / commanddotnet

A modern framework for building modern CLI apps
https://commanddotnet.bilal-fazlani.com
MIT License
570 stars 29 forks source link

When using MicrosoftDependencyInjection the ExceptionHandler is always broken #331

Closed arendvw closed 3 years ago

arendvw commented 3 years ago

When using MicrosoftDependencyInjection in combination with dotnet Framework 4.7.2 of 4.8 any exception occurring (and its stacktrace) will be obscured by a secondary ArgumentException,

Argument passed in is not serializable.
Parameter name: value

Caused by trying to serialize the CommandContext in ExceptionExtensions.cs, line 17

It seems microsoft's ServiceProvider is not serializable.

Minimum example to reproduce:

class MyJobRunner
{

    [Command(Description = "Run interactive job runner", Name = "run")]
    public async Task Run(IConsole console)
    {
        throw new Exception("Test");
    }
}

class Program
{
    static int Main(string[] args)
    {
        IServiceCollection serviceCollection = new ServiceCollection();
            serviceCollection.AddSingleton<MyJobRunner>();
            var provider = serviceCollection.BuildServiceProvider();
            var result = new AppRunner<MyJobRunner>()
                .UseDefaultMiddleware()
                .UseMicrosoftDependencyInjection(provider)
                .Run("run");
            return result;
        }
    }
}

Crude workaround is to wrap line 17 of ExeptionExtensions.cs in try catch clauses.

try
{
    ex.Data[typeof(CommandContext)] = ctx;
}
catch (ArgumentException)
{
    // argument passed is not serializable..
}

I'm currently not aware what the impact is of not setting the CommandContext in the exception. Possibly certain error handlers that can give less contextual information?

drewburlingame commented 3 years ago

Hi @arendvw, thanks for posting this.

We have this test which configures the MS ServiceProvider and the CommandContext is added to ex.Data successfully.

What dotnet framework are you targeting? I seem to remember having this issue with the pattern on older frameworks.

drewburlingame commented 3 years ago

@arendvw can you try https://www.nuget.org/packages/CommandDotNet/4.1.4-fix331. I pulled a hack from an old repo I had to address this on .Net Framework.

arendvw commented 3 years ago

@drewburlingame thanks for the quick reply!

We have this test which configures the MS ServiceProvider and the CommandContext is added to ex.Data successfully.

I will try to get the test suite running to get this to reproduce.

What dotnet framework are you targeting? I seem to remember having this issue with the pattern on older frameworks.

I'll try to get the test working on my computer, I've currently tried targeting .net framework 4.7.2 and .net framework 4.8

@arendvw can you try https://www.nuget.org/packages/CommandDotNet/4.1.4-fix331. I pulled a hack from an old repo I had to address this on .Net Framework

This fix seems to solve the issue for me, thanks a lot!

drewburlingame commented 3 years ago

Oops, didn't mean to close it yet. Forgot Github would do it when I merged the branch.

Glad the fix worked for you. This is an issue with .net framework. The serializable requirement was to support remoting. Since .net core doesn't support remoting, items added to ex.Data do not need to be serializable.

I'll push the new package when the build has completed.

arendvw commented 3 years ago

Glad the fix worked for you. This is an issue with .net framework. The serializable requirement was to support remoting. Since .net core doesn't support remoting, items added to ex.Data do not need to be serializable.

I'll push the new package when the build has completed.

Great, thanks a lot! I'll work on a pull request to get the tests to play nice with .net framework (almost there).