ninject / Ninject.Web.WebApi

Adds support for ASP.NET Web API to Ninject
Other
45 stars 37 forks source link

Incorrect implementation for WebApi with Owin (do not use this extension for the time being!) #22

Closed MoonStorm closed 4 years ago

MoonStorm commented 8 years ago

The extension has severe bugs when it comes to dealing with Owin. I would strongly recommend everyone to stay away from this until these bugs have been fixed.

The OwinNinjectDependencyResolver creates a named scope but it never uses it for resolving via HttpConfiguration.DependencyResolver. It's not proxying the calls to the proper resolution root, and it fails silently in OwinWebApiRequestScopeProvider with an UnknownScopeException, returning a NULL request scope. InRequestScope instances resolved like this will behave as transient and will not be cleaned up!

The implementation is also incompatible with Owin middleware pipeline. The Ninject.Web.Common.OwinHost creates a scope for this purpose inside OwinBootstrapper, however this extension completely ignores it. When attempting to resolve services inside middlewares, it behaves exactly the same as described above, failing silently and creating transient instances instead.

In a nutshell, the extension fails to properly cope with an Owin request scope. Using it can have severe consequences leading to unexpected situations which are very hard to detect and solve. I can't stress enough. This extension is NOT ready for prime time!

MoonStorm commented 8 years ago

An InRequestScope service should resolve into the same instance for an Owin context, regardless where that resolve occurs (same as it did before for an HTTP context).

After thinking about it, the fix should actually live inside Ninject.Web.Common.OwinHost, not in this extension. When the Ninject Owin Middleware gets set up it should create a single request scope (already doing that but unfortunately attaches the new scope to the Owin context - it is not easy to get it out of there from a Ninject component). This step should be the one responsible for setting up the dependency resolver and use the scope for any resolves past this point. No other scopes named or otherwise are needed. They'll do more harm than good, as we've already seen.

micktion commented 8 years ago

Is this related to ...

https://github.com/ninject/Ninject.Web.WebApi/issues/23

?

MoonStorm commented 8 years ago

Sort of. You shouldn't resolve them directly via the kernel. However in your case you do have a solution, as you can easily get them resolved in the constructor of the WebApi controller.

In case of Owin middlewares and dependency resolvers, this extension is unbelievably flawed though.

We've had to shut down a Web Forms site in production, which was using Owin mainly for authentication, due to memory leaks. I thought that with all its faults, I would at least get transient instances for InRequestScope services that would be GCed eventually, but after a few sessions of PerfView inspections of crash dumps after several OutOfMemoryExceptions on a 32 bit app, these resolved instances were actually being cached and never released.

My immediate solution was to remove the Ninject Owin package from our app and just use its web common package. That's using the HttpContext as the scope. This was for a Web Forms app though.

On all the other apps I'm working on, including a Web Api app with custom Owin middlewares, I simply switched to a different DI framework.

rmaziarka commented 8 years ago

@MoonStorm i dont get, why instead of trying to change the Ninject.Web.WebApi sources and fix that problem you moved to other DI framework. You clearly wrote whole description what is going wrong, and you decided to leave it. It seems that fixing it will be much easier than changing to other solution.

MoonStorm commented 8 years ago

@rmaziarka Believe me I tried, but i failed. Hopefully my findings would prove useful to anyone with more willpower in finding a fix.

micktion commented 8 years ago

I've not looked at the code for NInject, but I suspect the main issues are due to the architectural changes made to the .NET web development stack. There are a lot of challenges to overcome for frameworks which heavily relied upon the old ASP.NET HTTP Stack. Anyone attempting to get NInject 100% operational within OWIN might find something like this useful...

https://github.com/danielcrenna/graveyard/tree/master/httpcontext-shim

... which is a replacement for the old HttpContext.Current...

foresightyj commented 8 years ago

@MoonStorm I also read the code just now and shared similar findings with you. This repository seems to be completely abandoned by Ninject. I am also looking for another DI and may I know which one you had good luck with finally?

micktion commented 8 years ago

@foresightyj I went with Autofac

nathanrobb commented 5 years ago

Ignore this: Instead make sure to install the Ninject.Extensions.ContextPreservation package. Refs #26


I tried using the UnitOfWorkScope package https://github.com/MoonStorm/Ninject.Extensions.UnitOfWork

This seemed to work. Perhaps this package could reuse some parts of that package with further OWIN integration to not require the manual app builder use below.

Startup.cs

app
    .UseUnitOfWorkScope()
    .UseNinjectMiddleware(() => new StandardKernel(new AppModule()))
    .UseNinjectWebApi(config);

AppModule.cs

public class AppModule : NinjectModule
{
    public override void Load()
    {
        Bind<IInterface>().To<Type>().InUnitOfWorkScope();
    }
}

UnitOfWorkScopeBuilder.cs

public static class UnitOfWorkScopeBuilder
{
    public static IAppBuilder UseUnitOfWorkScope(this IAppBuilder app)
    {
        return app.Use(async (ctx, next) =>
        {
            using (UnitOfWorkScope.Create())
            {
                await next();
            }
        });
    }
}

Full disclosure, I have not done any throughput or async testing with the UnitOfWorkScope package.

scott-xu commented 4 years ago

Ninject.Web.WebApi 3.3.1 fixed the issue. See the sample applications please.