umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.42k stars 2.67k forks source link

UmbracoHttpHandler abstract class has null UmbracoHelper injected #6018

Closed marcemarc closed 5 years ago

marcemarc commented 5 years ago

Umbraco provides several abstract base classes for developers to extend for convenience and to expose properties such as UmbracoHelper, ServiceContext, Logger etc without the need to wire up DI to have them injected. One such abstract base class is the UmbracoHttpHandler

https://github.com/umbraco/Umbraco-CMS/blob/853087a75044b814df458457dc9a1f778cc89749/src/Umbraco.Web/UmbracoHttpHandler.cs

This is presumably provided to allow developers to have a convenient starting point for their custom HttpHandlers that need to access Umbraco resources, and sometimes these abstract classes are 'type scanned' in the WebInitialComposer so they are automatically registered with DI. This isn't the case for handlers that inherit UmbracoHttpHandler.

if you register your own handler that extends UmbracoHttpHandler,

  public class RegisterCustomHttpHandler : IUserComposer
    {
        public void Compose(Composition composition)
        {
            composition.Register<CustomHttpHandler>(Lifetime.Request);
        }
    }

and you have a constructor for your custom httphandler that accepts all the resources that the abstract UmbracoHttpHandler requires:

    public  CustomHttpHandler(IUmbracoContextAccessor umbracoContextAccessor, UmbracoHelper umbracoHelper, ServiceContext service, IProfilingLogger plogger) : base(umbracoContextAccessor, umbracoHelper, service, plogger)
        {
        }

then you will get an error:

[NullReferenceException: Object reference not set to an instance of an object.] Umbraco.Web.Runtime.<>c.b0_6(IFactory factory) in d:\a\1\s\src\Umbraco.Web\Runtime\WebInitialComposer.cs:117 Umbraco.Core.Composing.LightInject.<>c__DisplayClass20_0`1.b0(IServiceFactory f) in d:\a\1\s\src\Umbraco.Core\Composing\LightInject\LightInjectContainer.cs:172 DynamicMethod(Object[] ) +61 LightInject.ServiceContainer.GetInstance(Type serviceType) in C:\projects\lightinject\src\LightInject\LightInject.cs:3442 Umbraco.Core.Composing.LightInject.LightInjectContainer.GetInstance(Type type) in d:\a\1\s\src\Umbraco.Core\Composing\LightInject\LightInjectContainer.cs:111 Umbraco.Core.FactoryExtensions.GetInstance(IFactory factory) in d:\a\1\s\src\Umbraco.Core\FactoryExtensions.cs:22 umbraco_8_1_release.Handlers.CustomHttpHandler..ctor() in G:\src\Umbraco-8-1-0\umbraco-8-1-release\umbraco-8-1-release\Handlers\CustomHttpHandler.cs:24

Essentially the UmbracoHelper is null...

If I revert to not using the abstract UmbracoHttpHandler, using IHttpHandler and try instead to use IUmbracoContextFactory and call EnsureUmbracoContext to be able to query the Umbraco cache...

then UmbracoContextFactory also is null for a request handled by the CustomHttpHandler...

[NullReferenceException: Object reference not set to an instance of an object.] umbraco_8_1_release.Handlers.CustomHttpHandler..ctor(IUmbracoContextFactory umbracoContextFactor) in G:\src\Umbraco-8-1-0\umbraco-8-1-release\umbraco-8-1-release\Handlers\CustomHttpHandler.cs:31 umbraco_8_1_release.Handlers.CustomHttpHandler..ctor() in G:\src\Umbraco-8-1-0\umbraco-8-1-release\umbraco-8-1-release\Handlers\CustomHttpHandler.cs:25

What is the implementation of the in full CustomHttpHandler? (have tried a few variations - but think this is how I'd expect it to work...)

    public class CustomHttpHandler : UmbracoHttpHandler
    {
        public CustomHttpHandler() : base(Current.Factory.GetInstance<IUmbracoContextAccessor>(), Current.Factory.GetInstance<UmbracoHelper>(), Current.Factory.GetInstance<ServiceContext>(), Current.Factory.GetInstance<IProfilingLogger>())
        {
        }
        public CustomHttpHandler(IUmbracoContextAccessor umbracoContextAccessor, UmbracoHelper umbracoHelper, ServiceContext service, IProfilingLogger plogger) : base(umbracoContextAccessor, umbracoHelper, service, plogger)
        {
        }

        public override bool IsReusable => false;

        public override void ProcessRequest(HttpContext context)
        {
            var rootContent = Umbraco.ContentAtRoot();
            HttpResponse Response = context.Response;
            Response.Write("<html><body>");
            Response.Write(rootContent?.FirstOrDefault().Name);
            Response.Write("This is a custom HttpHandler");
            Response.Write("</body></html>");
        }
    }
}

It's possible the UmbracoHttpHelper is just a legacy thing... so just knowing 'don't use this' - this will never work - is also useful!


_This item has been added to our backlog AB#2314_

nul800sebastiaan commented 5 years ago

Not sure what this means Marc, I'll ask around at HQ!

Shazwazza commented 5 years ago

Hi @marcemarc OOTB when we shipped v8, webforms and friends such as httphandlers are not DI compatible. However, with the .net framework version we are using: net472, you can do this: https://devblogs.microsoft.com/aspnet/use-dependency-injection-in-webforms-application/

But, this has not been tested at all and I don't even know if LightInject has a provider (probably not). It would seem to wire that up requires setting this HttpRuntime.WebObjectActivator = someProvider;, a more detailed post is here https://www.natmarchand.fr/aspnet-dependency-injection-net472/

We don't intend on adding support for this though since we don't really plan on supporting anything to do with WebForms at this stage.

The reason why these base classes exists is so that you don't have to manually have to use the service locator pattern. If you use the base class: UmbracoHttpHandler and just use the empty ctor, these services are populated by the Current... service locator. The reason why the full ctor's exist is so you can still unit test these if you wanted.

So unless you are manually wiring up the support in net472 for DI for webforms based stuff, then you shouldn't be registering your httphandlers or other webforms things with the controller since that's not going to work.

You can see the base class empty ctor here: https://github.com/umbraco/Umbraco-CMS/blob/v8/dev/src/Umbraco.Web/UmbracoHttpHandler.cs#L17

That all said, if you still get a null ref, it could be because the handler is being created before an HttpContext and therefore LightInject isnt' returning anything for UmbracoHelper as a request based lifetime? Not sure, can you see if you still get null by using one of these normally? (i.e. no DI, empty ctor)

Zweben commented 5 years ago

That all said, if you still get a null ref, it could be because the handler is being created before an HttpContext and therefore LightInject isnt' returning anything for UmbracoHelper as a request based lifetime? Not sure, can you see if you still get null by using one of these normally? (i.e. no DI, empty ctor)

@Shazwazza - I have been trying to implement UmbracoHttpHandler normally, without using DI, and with no ctor specified. Even with a bare-bones implementation, I get a NullReferenceException before any breakpoints inside the class are hit. Example code below:

using System.Web;
using Umbraco.Web;

public class MediaHttpHandler : UmbracoHttpHandler
{
    public override bool IsReusable => true;

    public override void ProcessRequest(HttpContext context)
    {
        throw new System.NotImplementedException();
    }
}

The same behavior happens for UmbracoAuthorizedHttpHandler.

As soon as I switch it to implement IHttpHandler instead, as shown below, it starts working normally:

using System.Web;
using Umbraco.Web;

public class MediaHttpHandler : IHttpHandler
{
    public bool IsReusable => true;

    public void ProcessRequest(HttpContext context)
    {
        throw new System.NotImplementedException();
    }
}

Here is the stack trace:

[NullReferenceException: Object reference not set to an instance of an object.]
   Umbraco.Web.Runtime.<>c.<Compose>b__0_6(IFactory factory) +23
   Umbraco.Core.Composing.LightInject.<>c__DisplayClass20_0`1.<Register>b__0(IServiceFactory f) +15
   DynamicMethod(Object[] ) +61
   LightInject.ServiceContainer.GetInstance(Type serviceType) +168
   Umbraco.Core.Composing.LightInject.LightInjectContainer.GetInstance(Type type) +10
   Umbraco.Core.FactoryExtensions.GetInstance(IFactory factory) +56
   Umbraco.Web.UmbracoHttpHandler..ctor() +32
   MyProject.Core.HttpHandlers.MediaHttpHandler..ctor() +42

[TargetInvocationException: Exception has been thrown by the target of an invocation.]
   System.RuntimeTypeHandle.CreateInstance(RuntimeType type, Boolean publicOnly, Boolean noCheck, Boolean& canBeCached, RuntimeMethodHandleInternal& ctor, Boolean& bNeedSecurityCheck) +0
   System.RuntimeType.CreateInstanceSlow(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, StackCrawlMark& stackMark) +119
   System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, StackCrawlMark& stackMark) +232
   System.Activator.CreateInstance(Type type, Boolean nonPublic) +83
   System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes, StackCrawlMark& stackMark) +1088
   System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes) +124
   System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture) +20
   System.Web.HttpRuntime.CreateNonPublicInstance(Type type, Object[] args) +59
   System.Web.HttpRuntime.CreateNonPublicInstanceByWebObjectActivator(Type type) +65
   System.Web.Configuration.HandlerFactoryCache..ctor(String type) +31
   System.Web.HttpApplication.GetFactory(String type) +78
   System.Web.MaterializeHandlerExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +290
   System.Web.HttpApplication.ExecuteStepImpl(IExecutionStep step) +132
   System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +163
marcemarc commented 5 years ago

@Shazwazza this follows a forum post here: https://our.umbraco.com/forum/umbraco-8/96270-using-umbracohelper-in-a-custom-class-in-v8#comment-310037

and yes accessing via the provided base class UmbracoHttpHandler properties without any DI is what was tried first, and also throws the null error, so it's likely the order in which the HttpHandler creation sits in respect to lightinject's service locator.

I think people still implement HttpHandlers, and the existence of UmbracoHttpHandler abstract class does suggest as a dev that this could be used as the base for creating new ones, and the UmbracoHelper property implies you should be able to access the cache here, but if not, then I'd suggest removing this starting off point... but making it work is also good!

Shazwazza commented 5 years ago

@marcemarc will need to investigate šŸ¤” First thing is that when using IsReusable = true in an http handler, it tells asp.net that it's a singleton which means only one instance of it will be created which means that no injected request based instances will actually work. This is probably part of the problem.

I think that perhaps injecting the UmbracoHelper into this is a mistake or can be done differently. There's a reason that we inject IUmbracoContextAccessor into the httphandler and that's because it's a singleton too and allows us to get the 'current' context from it.

In the case of UmbracoHttpHandler it is probable that the UrlHelper Url property is not working as expected either because if the http handler is treated as a singleton in some cases it means that it's capturing the current request for the lifetime of the app too.

Can you please test when using public bool IsReusable => false; and see if everything works? In theory it should because when this is false a new instance of the handler is created for each request.

That said, we will need to cater for what we do when IsReusable => true

marcemarc commented 5 years ago

@Shazwazza sorry to report I have tried both true and false for IsReusable and the error is still the same. I think the below is what we are saying 'should work'

namespace umbraco_8_1_release.Handlers
{
    public class CustomHttpHandler : UmbracoHttpHandler
    {
        public override bool IsReusable => false;

        public override void ProcessRequest(HttpContext context)
        {
            var rootContent = Umbraco.ContentAtRoot();
            HttpResponse Response = context.Response;
            Response.Write("<html><body>");
            Response.Write(rootContent?.FirstOrDefault().Name);
            Response.Write("This is a custom HttpHandler");
            Response.Write("</body></html>");
        }
    }
}

with handler <add name="CustomHttpHandler" verb="*" path="*.shaz" type="umbraco_8_1_release.Handlers.CustomHttpHandler"/>

but when requesting /wazza.shaz

the following error is thrown (this is before the ProcessRequest is fired)

[NullReferenceException: Object reference not set to an instance of an object.] Umbraco.Web.Runtime.<>c.b0_6(IFactory factory) in d:\a\1\s\src\Umbraco.Web\Runtime\WebInitialComposer.cs:117 Umbraco.Core.Composing.LightInject.<>c__DisplayClass20_0`1.b0(IServiceFactory f) in d:\a\1\s\src\Umbraco.Core\Composing\LightInject\LightInjectContainer.cs:172 DynamicMethod(Object[] ) +61 LightInject.ServiceContainer.GetInstance(Type serviceType) in C:\projects\lightinject\src\LightInject\LightInject.cs:3442 Umbraco.Core.Composing.LightInject.LightInjectContainer.GetInstance(Type type) in d:\a\1\s\src\Umbraco.Core\Composing\LightInject\LightInjectContainer.cs:111 Umbraco.Core.FactoryExtensions.GetInstance(IFactory factory) in d:\a\1\s\src\Umbraco.Core\FactoryExtensions.cs:22 Umbraco.Web.UmbracoHttpHandler..ctor() in d:\a\1\s\src\Umbraco.Web\UmbracoHttpHandler.cs:17 umbraco_8_1_release.Handlers.CustomHttpHandler..ctor() +42

If I inherit from IHttpHandler (and remove the Umbraco specific stuff) then the request is handled without error, it's only when I inherit from UmbracoHttpHandler that the error occurs.

    protected UmbracoHttpHandler()
            : this(Current.UmbracoContextAccessor, Current.UmbracoHelper, Current.Services, Current.ProfilingLogger)
        { }

        protected UmbracoHttpHandler(IUmbracoContextAccessor umbracoContextAccessor, UmbracoHelper umbracoHelper, ServiceContext service, IProfilingLogger plogger)
        {
            UmbracoContextAccessor = umbracoContextAccessor;
            Logger = plogger;
            ProfilingLogger = plogger;
            Services = service;
            Umbraco = umbracoHelper;
        }

So I agree, it is the Current.UmbracoHelper that is causing the issue but I can't see the condition which means the httphandler request causes the problem in the WebInitialComposer: https://github.com/umbraco/Umbraco-CMS/blob/17f73b88dc0c057054703ab15e9d84013021ee1b/src/Umbraco.Web/Runtime/WebInitialComposer.cs#L116 :

var umbCtx = factory.GetInstance(); return new UmbracoHelper(umbCtx.IsFrontEndUmbracoRequest ? umbCtx.PublishedRequest?.PublishedContent : null,

with the UmbracoContext being null.

but you know me, I'm equally likely to be doing something stupid!

Zweben commented 5 years ago

@Shazwazza I can confirm marcemarc's result. I set IsResuable => false on my code, and I got the same result of a NullReferenceException as when IsReusable was true.

Is there a way we can work around the lack of a working IUmbracoHttpHandler by accessing Umbraco services from an IHttpHandler, via DI or otherwise? I need to set up access restrictions on certain static file HTTP requests, and I am stumped as to how I can achieve this at the moment.

Shazwazza commented 5 years ago

Yes so the reason for the null ref isn't because of the UmbracoHelper, it's because of the UmbracoContext. An UmbracoContext is not created for every single request, we detect if one should be built (this was the same in v7 but maybe some other odd stuff was working).

The code is in the UmbracoInjectedModule where we create the context but before that we do this check:

            // do not process if client-side request
            if (httpContext.Request.Url.IsClientSideRequest())
                return;

The IsClientSideRequest is reasonably simple, if there is no extension or, its: ".aspx", ".ashx", ".asmx", ".axd", ".svc" then it's a server side request, else it's client side.

So please try again using an axd extension or one of the above.

You will need to use IsReusable = false, otherwise you are going to get an 'object disposed' exception.

I have tested this and it works, however the UmbracoHttpHandler should have IsReusable = false and probably seal it (but that would be breaking :/ ) so we'll have to just deal with setting it to false and leaving that property as virtual.

If you want to use an odd extension, the you are going to have to jump through some hoops and not use the UmbracoHttpHandler and just use your own IHttpHandler and then use the IUmbracoContextFactory and resolve the content cache directly from the umbraco context (you don't need the helper to do simple cache retrieval), else you'll need to manually construct an UmbracoHelper.

IHttpHandlers are legacy, you should be able to achieve what you want with custom MVC routes, if you really like IHttpHandlers you can even use them in routes.

... but at the end of the day, custom routes won't solve the issue either since any custom extension isn't going to automatically get an UmbracoContext created for it.

So how to solve this? The work around for now will be to create a custom IHttpModule and bind to the BeginRequest method, check for your custom extensions to see if an UmbracoContext should be created and then do:

var umbracoContextReference = _umbracoContextFactory.EnsureUmbracoContext(httpContext);
httpContext.DisposeOnPipelineCompleted(umbracoContextReference);

Else the IsClientSideRequest will need to be changed and somehow allowed to be extended - this method is used for quite a lot of things and if this passes, then things like authz/authn are not executed which might affect what you are doing with custom extensions as well. This same logic exists in all of v7 too.

marcemarc commented 5 years ago

That works with the .axd extension, and makes sense, it was interesting to try and see what was going on... will update the forum post where the attempt is to handle a request to style.theme but no reason that shouldn't be style.axd

thanks!

LennardF1989 commented 4 years ago

Just randomly throwing this out there: Could it be UmbracoHelper (and particularly Current.UmbracoHelper) is not thread-safe?

I run into the same issue when trying to do a parallelized import job that triggers MediaService.Save, which in turn triggers one of my hooks that uses Current.UmbracoHelper. When I do this from a direct request (no Task.Run), Current.UmbracoHelper works fine. If I use it in the context of Task.Run, Current.UmbracoHelper gets the same NullpointerException as above (the Umbraco.Web.Runtime one - see below).

Seeing this, it could also explain the above seeing how IIS handles HttpHandlers and requests in general.

   at Umbraco.Web.Runtime.WebInitialComposer.<>c.<Compose>b__0_6(IFactory factory)
   at Umbraco.Core.FactoryExtensions.GetInstance[T](IFactory factory)

EDIT: So I recreated the UmbracoHelper registration in the offending part of my code:

            var umbCtx = Current.Factory.GetInstance<UmbracoContext>();
            var tagQuery = Current.Factory.GetInstance<ITagQuery>();
            var cultureDictionary = Current.Factory.GetInstance<ICultureDictionaryFactory>();
            var componentRenderer = Current.Factory.GetInstance<IUmbracoComponentRenderer>();
            var publishedContentQuery = Current.Factory.GetInstance<IPublishedContentQuery>();
            var membershipHelper = Current.Factory.GetInstance<MembershipHelper>();

            var umbracoHelper = new UmbracoHelper(
                umbCtx.IsFrontEndUmbracoRequest ? umbCtx.PublishedRequest?.PublishedContent : null,
                tagQuery, 
                cultureDictionary,
                componentRenderer,
                publishedContentQuery,
                membershipHelper
            );

It gets past the UmbracoContext, but will return a null. Then the ITagQuery comes around and it throws the following exception:

   at LightInject.Web.PerWebRequestScopeManager.GetOrAddScope()
   at Umbraco.Core.FactoryExtensions.GetInstance[T](IFactory factory)

I'm pretty sure it's a thread-releated issue now. Obviously my job is running long after the request has ended. While it doesn't immediately explain the null for the UmbracoContext, it can explain why a Request-scoped instance fails in a thread that isn't actually tied to a request.

EDIT EDIT: I did something very nasty in my import job:

            while (activeThreads > 0)
            {
                Thread.Sleep(100);
            }

This however does not actually fix anything, so it looks like that whenever you do something with Current in another thread than the one that started the job, things go haywire.

LennardF1989 commented 4 years ago

Made a reproduction for the above:

    public class TestController : ApiController
    {
        [HttpGet]
        public void Repro()
        {
            //Works!
            var umbCtx = Current.Factory.GetInstance<UmbracoContext>();
            var tagQuery = Current.Factory.GetInstance<ITagQuery>();

            Task.Run(() =>
            {
                //Doesn't work
                var umbCtx1 = Current.Factory.GetInstance<UmbracoContext>();
                var tagQuery1 = Current.Factory.GetInstance<ITagQuery>();
            });
        }
    }

@nul800sebastiaan Can this issue be reopened, or should I create a new one. I feel it's similar, but also hijack-y.

LennardF1989 commented 4 years ago

Played a little more with this. UmbracoHelper is simply evil to use in the context of threads.

Instead of Task.Run I moved over to HostingEnvironment.QueueBackgroundWorkItem and did using var _ = umbracoContextFactory.EnsureUmbracoContext(); right before doing whatever my job is doing. Then instead of the offending Current.UmbracoHelper I moved to Current.UmbracoContext.Media to do my things with published media.

I haven't tested any further, but even with a valid UmbracoContext, ITagQuery appears to fail to resolve, which seems mostly related to the fact they are scoped to the Request lifetime. EnsureUmbracoContext will mimic being a request if it was started in a thread, yet, it doesn't seem to completely fool LightInject in this regard... So maybe that is the real issue.

Shazwazza commented 4 years ago

Hi all,

UmbracoContext is a request based lifetime. Anything that depends on it like UmbracoHelper is therefore also a request based lifetime. If there is no request, there is no UmbracoContext. There's some docs here https://our.umbraco.com/documentation/Implementation/Services/#accessing-published-content-outside-of-a-http-request There is also IUmbracoContextAccessor which is a singleton. This gives you the ability to see if an UmbracoContext is available or not.

UmbracoContext must be disposed at the end of operation otherwise bad things will happen. So if you are using UmbracoContextFactory be sure that you aren't pinning an UmbracoContext instance to a static or singleton.

LennardF1989 commented 4 years ago

@Shazwazza Thanks for clarifying. EnsureUmbracoContext attempts to mimic a request if there is actually none (eg. in the context of a scheduled job in a thread). However, this doesn't actually seem to "fool" LightInject Request-scoped objects (eg. ITagQuery, which makes UmbracoHelper fail, among other things). Is this something that can be fixed at all? Eg. Can we actually "fool" LightInject into believing there was a request so Request-scoped objects work too, or is it simply a limitation of a lot of things combined and we just have to accept that some things just don't work without an actual request?

Shazwazza commented 4 years ago

@LennardF1989 No you cannot 'fool' the container to think there is a request to resolve a service in a non-request. For request based objects that you would like to manage yourself because they should not exist as singletons you will need to construct this yourself and be sure to manage their lifetimes appropriately by disposing them when you are done (do not pin these objects to statics or singletons)