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

Enable Automatically perform verification upon first resolve by default #747

Closed
dotnetjunkie opened this issue Sep 4, 2019 · 3 comments
Closed

Comments

@dotnetjunkie
Copy link
Collaborator

dotnetjunkie commented Sep 4, 2019

The implemented feature of #555 should be enabled by default in v5. Documentation can be found here.

@dotnetjunkie
Copy link
Collaborator Author

dotnetjunkie commented Apr 27, 2020

With the latest changes in the integration packages for ASP.NET Core 3, it became a little more consequential to switch this feature on by default.

In ASP.NET Core 3, hosted services can be resolved from Simple Injector by the framework before the Configure method runs. Middleware, on the other hand, is added to Simple Injector and the request pipeline from inside the Configure method. Middleware can't be added to the pipeline in an earlier stage, because the order in which middleware is added is crucial. The trick that is applied here, is that the integration package creates an InstanceProducer. InstanceProducers can be created at any time, even after the Container was locked.

The advantage of this is that Middleware can be added to the request pipeline (and registered at that same time), while allowing Verify() to be called after that.

But here comes the problem. When we enable automatic verification in Simple Injector, it will cause the container to be verified before the Configure method/phase. Although middleware can still be added, it will likely cause the user to remove that second (explicit) call to Verify, because the container has already been validated. Problem being that the container was validated without the middleware.

The following shows a trimmed down version of the Startup class which demonstrates this:

    public void ConfigureServices(IServiceCollection services)
    {
        services.AddControllersWithViews();

        services.AddSimpleInjector(container, options =>
        {
            options.AddAspNetCore().AddControllerActivation();
            options.AddHostedService<MyHostedService>();
        });
    }
    // MyHostedService gets resolved before Configure is called
    public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
    {
        app.UseSimpleInjector(container);

        // Default ASP.NET middleware
        app.UseStaticFiles();
        app.UseRouting();

        // UseMiddleware creates a new registration (InstanceProducer)
        app.UseMiddleware<CustomMiddleware1>(container);

        app.UseEndpoints(...);

        // Call verify after UseMiddleware
        container.Verify();
    }

@dotnetjunkie
Copy link
Collaborator Author

dotnetjunkie commented May 28, 2020

Another, and perhaps even more important, issue with this change is when Simple Injector is used inside integration tests.

A very common practice is to build a new container instance for each integration test (by reusing the same configuration logic, while perhaps replacing a few test specific dependencies). Configuration the container is reasonably quickly, especially when all assemblies already have been loaded. In that case, however, you would normally not call Verify() before each test, because of the (quite severe) performance implications that this will have on your test suite for bigger applications. Instead, users would typically have one single (integration) test that calls Container.Verify() to ensure the configuration is valid.

What this means is that unknowing users might upgrade to v5 and they might even do so without having to make any changes to their source code. But once the upgrade is committed and picketed up by the build server, their integration tests suddenly become very slow. But such delay will usually not be noticed at first, because build servers are typically quite unreliable. So once developers start to notice that the tests now run much more slowly, it will be more difficult to trace what is causing that delay.

dotnetjunkie added a commit that referenced this issue May 28, 2020
…esolve exception message to become confusing and wrong. Removed that extra information. #747
@dotnetjunkie
Copy link
Collaborator Author

Still, I find this feature to be very valuable, especially for newer users, but even for experienced users that start a new project. Verify is forgotten to be called more often than not. Over time, developers will start to understand the new behavior, and how to disable auto verification. In the long run having this feature enabled is our best pick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant