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

Fixing #7 #8

Merged
merged 3 commits into from Oct 17, 2012

Conversation

Projects
None yet
3 participants
@asgerhallas
Contributor

asgerhallas commented Sep 19, 2012

I've changed the bootstrapper to be independent on the number and/or order of calls to RegisterTypes and RegisterCollectionTypes.

The need for this is shown by adding an ApplicationRegistrationTask that will be picked up by Initialize() which in turn calls RegisterTypes twice. The old implementation threw.

It's somewhat hacky to change the ComponentModels directly, but at least contained.

For some reason the name of the proxy for INancyEngine changed with this new implementation. It still proxies as it should, so it seems ok?

@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Sep 21, 2012

Contributor

Could this be of use?

Contributor

asgerhallas commented Sep 21, 2012

Could this be of use?

Show outdated Hide outdated src/Nancy.BootStrappers.Windsor.Tests/ApplicationRegistrationTask.cs
@@ -0,0 +1,34 @@
using System.Collections.Generic;

This comment has been minimized.

@thecodejunkie

thecodejunkie Sep 21, 2012

Member

Using statements should be placed inside the namespace declaration

@thecodejunkie

thecodejunkie Sep 21, 2012

Member

Using statements should be placed inside the namespace declaration

Show outdated Hide outdated src/Nancy.BootStrappers.Windsor/WindsorNancyBootstrapper.cs
@@ -1,3 +1,5 @@
using Castle.Facilities.TypedFactory;

This comment has been minimized.

@thecodejunkie

thecodejunkie Sep 21, 2012

Member

Using statements should be place inside the namespace declaration

@thecodejunkie

thecodejunkie Sep 21, 2012

Member

Using statements should be place inside the namespace declaration

Show outdated Hide outdated src/Nancy.BootStrappers.Windsor/WindsorNancyBootstrapper.cs
container.Register(Component.For(servicesFromCollectionTypes.Union(servicesFromTypes))
.ImplementedBy(implementationType).LifeStyle.Singleton);
static void RegisterNewOrAddService(IWindsorContainer container, Type registrationType, Type implementationType)

This comment has been minimized.

@thecodejunkie

thecodejunkie Sep 21, 2012

Member

Missing the private access modifier

@thecodejunkie

thecodejunkie Sep 21, 2012

Member

Missing the private access modifier

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie Sep 21, 2012

Member

@asgerhallas I'm sure it will :) I've added some code convention review comments and will review the functionality as soon as possible. We're currently looking over the 30+ open pull-requests on the main Nancy repository. Thank you!

Member

thecodejunkie commented Sep 21, 2012

@asgerhallas I'm sure it will :) I've added some code convention review comments and will review the functionality as soon as possible. We're currently looking over the 30+ open pull-requests on the main Nancy repository. Thank you!

@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Sep 21, 2012

Contributor

@thecodejunkie Very nice, I'll fix those violations asap. And BTW I really did not mean to be pushy, I can imagine you have a lot going on :)

Contributor

asgerhallas commented Sep 21, 2012

@thecodejunkie Very nice, I'll fix those violations asap. And BTW I really did not mean to be pushy, I can imagine you have a lot going on :)

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 1, 2012

@asgerhallas I'm curious about your fix and whether it can help me with another bug I have revealed. I use an AbstractFacility to add a Logging interceptor to all my dependencies. After upgrading to Nancy 0.12 (and appropriately upgrading to Castle 3.1 and the new windsor bootstrapper) I'm having a problem because of the following line in WindsorNancyBootstrapper:

        container.Register(Component.For<Func<IRouteCache>>()
            .UsingFactoryMethod(ctx => (Func<IRouteCache>) (ctx.Resolve<IRouteCache>)));

It seems that Castle tries to wrap a proxy around "Func" and can't. I can't seem to be able to get it to ignore this interception...

nikp commented Oct 1, 2012

@asgerhallas I'm curious about your fix and whether it can help me with another bug I have revealed. I use an AbstractFacility to add a Logging interceptor to all my dependencies. After upgrading to Nancy 0.12 (and appropriately upgrading to Castle 3.1 and the new windsor bootstrapper) I'm having a problem because of the following line in WindsorNancyBootstrapper:

        container.Register(Component.For<Func<IRouteCache>>()
            .UsingFactoryMethod(ctx => (Func<IRouteCache>) (ctx.Resolve<IRouteCache>)));

It seems that Castle tries to wrap a proxy around "Func" and can't. I can't seem to be able to get it to ignore this interception...

@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Oct 2, 2012

Contributor

Yes, I believe this fix will solve that. The problem here, is that registering a Func like this in Windsor actually is the wrong way to do it. Given Windsors' TypedFactoryFacility, it does per default give you a Func of the given type, if the given service requires a Func in the constructor.

The explicit registered Func (without an .AsFactory call) will be seen as any other registered type and thus be wrapped in your intercepter as is.

Contributor

asgerhallas commented Oct 2, 2012

Yes, I believe this fix will solve that. The problem here, is that registering a Func like this in Windsor actually is the wrong way to do it. Given Windsors' TypedFactoryFacility, it does per default give you a Func of the given type, if the given service requires a Func in the constructor.

The explicit registered Func (without an .AsFactory call) will be seen as any other registered type and thus be wrapped in your intercepter as is.

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 2, 2012

Interesting. Let me try your latest version and see if it resolves my issue.

nikp commented Oct 2, 2012

Interesting. Let me try your latest version and see if it resolves my issue.

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 2, 2012

@asgerhallas, just tried your change. Unfortunately it does not work with the interceptor facility. What seems to happen now is I get an exception on return this.ApplicationContainer.Resolve<NancyModule>(moduleKey); in GetModuleByKey:

ComponentNotFoundException:
Requested component named 'Castle.Proxies.Proxy' was not found in the container. Did you forget to register it?
There are 42 other components supporting requested service 'Nancy.NancyModule'. Were you looking for any of them?

It seems that the Nancy modules themselves end up getting proxied and intercepted, which is not desired behaviour, and doesn't happen with the current windsor nancy bootstrapper

nikp commented Oct 2, 2012

@asgerhallas, just tried your change. Unfortunately it does not work with the interceptor facility. What seems to happen now is I get an exception on return this.ApplicationContainer.Resolve<NancyModule>(moduleKey); in GetModuleByKey:

ComponentNotFoundException:
Requested component named 'Castle.Proxies.Proxy' was not found in the container. Did you forget to register it?
There are 42 other components supporting requested service 'Nancy.NancyModule'. Were you looking for any of them?

It seems that the Nancy modules themselves end up getting proxied and intercepted, which is not desired behaviour, and doesn't happen with the current windsor nancy bootstrapper

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 2, 2012

Upon further investigation, I have control over this. The fix may work after all!

nikp commented Oct 2, 2012

Upon further investigation, I have control over this. The fix may work after all!

@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Oct 2, 2012

Contributor

@nikp Oh, sounds good after all :)
Could you elaborate on what went wrong? I'm not sure why the moduleKey would be "Castle.Proxies.Proxy", but then again I might not know the details on how Nancy does some of it's things.

Contributor

asgerhallas commented Oct 2, 2012

@nikp Oh, sounds good after all :)
Could you elaborate on what went wrong? I'm not sure why the moduleKey would be "Castle.Proxies.Proxy", but then again I might not know the details on how Nancy does some of it's things.

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 2, 2012

Digging deeper now. Previously the Kernel.ComponentRegistered event in my Logging Facility never got triggered for any Nancy internals. After adding the facility Castle.Facilities.TypedFactory.TypedFactoryFacility, and switching to your version of the bootstrapper now all of them ARE, including my original problem with Func being proxied.

Next step: figure out if i can hold out with just using current 0.12 as long as I have the TypedFactoryFacility

nikp commented Oct 2, 2012

Digging deeper now. Previously the Kernel.ComponentRegistered event in my Logging Facility never got triggered for any Nancy internals. After adding the facility Castle.Facilities.TypedFactory.TypedFactoryFacility, and switching to your version of the bootstrapper now all of them ARE, including my original problem with Func being proxied.

Next step: figure out if i can hold out with just using current 0.12 as long as I have the TypedFactoryFacility

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie Oct 2, 2012

Member

@asgerhallas @nikp I was planning on reviewing and pulling this in tonight or tomorrow - Just wanted to double check with you guys that it's ok to pull in and that you've not found any other problems?

Member

thecodejunkie commented Oct 2, 2012

@asgerhallas @nikp I was planning on reviewing and pulling this in tonight or tomorrow - Just wanted to double check with you guys that it's ok to pull in and that you've not found any other problems?

@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Oct 2, 2012

Contributor

@nikp I'm not sure I follow... using this pull request, you are getting a Func proxied? Can you see where that Func is being registered?

@thecodejunkie We do use it in production right now without problems, but I guess I have to look into this new info.

Contributor

asgerhallas commented Oct 2, 2012

@nikp I'm not sure I follow... using this pull request, you are getting a Func proxied? Can you see where that Func is being registered?

@thecodejunkie We do use it in production right now without problems, but I guess I have to look into this new info.

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie Oct 2, 2012

Member

@asgerhallas thanks for the update. Sounds like it would be a second pull request if there is another fix, instead of growing on this one?

Member

thecodejunkie commented Oct 2, 2012

@asgerhallas thanks for the update. Sounds like it would be a second pull request if there is another fix, instead of growing on this one?

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 2, 2012

Not 100% in the clear. Just double-checked and with default Nancy, i was proxying all my modules via the logging facility successfully

However with this change, it seems to fail with the ComponentNotFoundException I described above. The bug I thought I discovered was fixed by registering the TypedFactoryFacility and ensuring in my Logging facility not to proxy Multicastdelegates. I will keep playing with this branch's change. (Full disclosure: I'm pretty n00b with both Nancy and Castle so please don't rely on me to yay/nay this change. It's above my paygrade)

nikp commented Oct 2, 2012

Not 100% in the clear. Just double-checked and with default Nancy, i was proxying all my modules via the logging facility successfully

However with this change, it seems to fail with the ComponentNotFoundException I described above. The bug I thought I discovered was fixed by registering the TypedFactoryFacility and ensuring in my Logging facility not to proxy Multicastdelegates. I will keep playing with this branch's change. (Full disclosure: I'm pretty n00b with both Nancy and Castle so please don't rely on me to yay/nay this change. It's above my paygrade)

@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Oct 2, 2012

Contributor

@thecodejunkie Yes, that might be better. I must say, I believe this is at least more functioning than before :)
EDIT: That is of course when using it against the recent Nancy changes.

Contributor

asgerhallas commented Oct 2, 2012

@thecodejunkie Yes, that might be better. I must say, I believe this is at least more functioning than before :)
EDIT: That is of course when using it against the recent Nancy changes.

@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Oct 2, 2012

Contributor

@nikp Ok. I do already register the TypedFactoryFacility in the bootstrapper, so it sounds strange, that this should make a difference. Is it possible for you to share some code, that does this, then I can try to see tomorrow if I can spot it?

Contributor

asgerhallas commented Oct 2, 2012

@nikp Ok. I do already register the TypedFactoryFacility in the bootstrapper, so it sounds strange, that this should make a difference. Is it possible for you to share some code, that does this, then I can try to see tomorrow if I can spot it?

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 2, 2012

@asgerhallas you're right I meant I register with TypedFactoryFacility with the 0.12 WindsorNancyBootstrapper. But it seems like both with that approach and with the bootstrapper from this pull request, I get `ComponentNotFoundException`` when trying to resolve the proxied version of the Module. This doesn't happen with 0.7/castle 2.5

If I explicitly exclude NancyModules from proxying for logging, everything seems to work...

nikp commented Oct 2, 2012

@asgerhallas you're right I meant I register with TypedFactoryFacility with the 0.12 WindsorNancyBootstrapper. But it seems like both with that approach and with the bootstrapper from this pull request, I get `ComponentNotFoundException`` when trying to resolve the proxied version of the Module. This doesn't happen with 0.7/castle 2.5

If I explicitly exclude NancyModules from proxying for logging, everything seems to work...

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 2, 2012

@asgerhallas More than happy to share code, which part would you like to see?

nikp commented Oct 2, 2012

@asgerhallas More than happy to share code, which part would you like to see?

@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Oct 3, 2012

Contributor

@nikp I think the code where you configure the container with the logging facility and proxying, might make me able to reproduce it.

Contributor

asgerhallas commented Oct 3, 2012

@nikp I think the code where you configure the container with the logging facility and proxying, might make me able to reproduce it.

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 3, 2012

Sure, here is the Bootstrapper and my facility.

public sealed class MyBootStrapper : WindsorNancyBootstrapper
{
    protected override void ConfigureApplicationContainer(IWindsorContainer existingContainer)
    {
        existingContainer.AddFacility<LoggingFacility>();
        existingContainer.Register(Component.For<LoggingInterceptor>());

        existingContainer.AddFacility<TypedFactoryFacility>();

        ...container dependent service registration
        ...container before/after/onerror filter registration
    }
    ...other configuration overrides
}

...

public class LoggingFacility : AbstractFacility 
{
    static readonly List<Type> TypesNotToIntercept = new List<Type>
    {
        typeof(IInterceptor),   //Don't intercept Interceptors
        typeof(MulticastDelegate),  //Func<> and the like
        typeof(LateBoundComponent), //Resolved with a factory, such as MongoDatabase
        typeof(NancyModule) //TODO: For some reason proxying NancyModules causes problems during registration after upgrade to 0.12 Can sacrifice this
    };

    protected override void Init() { Kernel.ComponentRegistered += KernelComponentRegistered; }

    static void KernelComponentRegistered(string key, IHandler handler)
    {
        if (!ShouldInterceptComponent(handler))
            return;

        // Don't add more than one logging interceptor to a component
        handler.ComponentModel.Interceptors.AddIfNotInCollection(InterceptorReference.ForType<LoggingInterceptor>());
    }

    static bool ShouldInterceptComponent(IHandler handler)
    {
        return !TypesNotToIntercept.Any(type => type.IsAssignableFrom(handler.ComponentModel.Implementation));
    }
}

nikp commented Oct 3, 2012

Sure, here is the Bootstrapper and my facility.

public sealed class MyBootStrapper : WindsorNancyBootstrapper
{
    protected override void ConfigureApplicationContainer(IWindsorContainer existingContainer)
    {
        existingContainer.AddFacility<LoggingFacility>();
        existingContainer.Register(Component.For<LoggingInterceptor>());

        existingContainer.AddFacility<TypedFactoryFacility>();

        ...container dependent service registration
        ...container before/after/onerror filter registration
    }
    ...other configuration overrides
}

...

public class LoggingFacility : AbstractFacility 
{
    static readonly List<Type> TypesNotToIntercept = new List<Type>
    {
        typeof(IInterceptor),   //Don't intercept Interceptors
        typeof(MulticastDelegate),  //Func<> and the like
        typeof(LateBoundComponent), //Resolved with a factory, such as MongoDatabase
        typeof(NancyModule) //TODO: For some reason proxying NancyModules causes problems during registration after upgrade to 0.12 Can sacrifice this
    };

    protected override void Init() { Kernel.ComponentRegistered += KernelComponentRegistered; }

    static void KernelComponentRegistered(string key, IHandler handler)
    {
        if (!ShouldInterceptComponent(handler))
            return;

        // Don't add more than one logging interceptor to a component
        handler.ComponentModel.Interceptors.AddIfNotInCollection(InterceptorReference.ForType<LoggingInterceptor>());
    }

    static bool ShouldInterceptComponent(IHandler handler)
    {
        return !TypesNotToIntercept.Any(type => type.IsAssignableFrom(handler.ComponentModel.Implementation));
    }
}
@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Oct 3, 2012

Contributor

@nikp @thecodejunkie I have tried several things now, and must admit I am not able to reproduce that error. One last thing, could you sent me a stack trace of the ComponentNotFoundException?

Contributor

asgerhallas commented Oct 3, 2012

@nikp @thecodejunkie I have tried several things now, and must admit I am not able to reproduce that error. One last thing, could you sent me a stack trace of the ComponentNotFoundException?

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie Oct 10, 2012

Member

@nikp @asgerhallas 0.13 is just around the corner and I am hoping to clear out the open Windosor issues asap. Could you give me an update on this? Safe to pull in or needs more investigation?

Member

thecodejunkie commented Oct 10, 2012

@nikp @asgerhallas 0.13 is just around the corner and I am hoping to clear out the open Windosor issues asap. Could you give me an update on this? Safe to pull in or needs more investigation?

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 10, 2012

I think this is safe to pull in. The TypedFactoryFacility registration should resolve most of the issues, but I could not tell you why.

@asgerhallas, I'm sorry I missed the notification for your last question. If you are still interested in considering this problem, I will update this with the stacktrace shortly

nikp commented Oct 10, 2012

I think this is safe to pull in. The TypedFactoryFacility registration should resolve most of the issues, but I could not tell you why.

@asgerhallas, I'm sorry I missed the notification for your last question. If you are still interested in considering this problem, I will update this with the stacktrace shortly

@nikp

This comment has been minimized.

Show comment
Hide comment
@nikp

nikp Oct 10, 2012

@asgerhallas the stacktrace: ComponentNotFoundException

Requested component named 'Castle.Proxies.RootModuleProxy' was not found in the container. Did you forget to register it?
There are 42 other components supporting requested service 'Nancy.NancyModule'. Were you looking for any of them?

at Castle.MicroKernel.DefaultKernel.Castle.MicroKernel.IKernelInternal.Resolve(String key, Type service, IDictionary arguments, IReleasePolicy policy)
at Castle.MicroKernel.DefaultKernel.Resolve(String key, Type service, IDictionary arguments)
at Castle.Windsor.WindsorContainer.Resolve[T](String key)
at Nancy.Bootstrappers.Windsor.WindsorNancyBootstrapper.GetModuleByKey(String moduleKey, NancyContext context)
at Nancy.Routing.DefaultRouteResolver.GetInitializedModuleForMatch(NancyContext context, Tuple4 routeMatchToReturn) at Nancy.Routing.DefaultRouteResolver.CreateRouteAndParametersFromMatch(NancyContext context, Tuple4 routeMatchToReturn)
at Nancy.Routing.DefaultRouteResolver.Resolve(String path, NancyContext context, IRouteCache routeCache)
at Nancy.Routing.DefaultRouteResolver.Resolve(NancyContext context)
at Nancy.Routing.DefaultRequestDispatcher.InvokeRouteResolver(NancyContext context, String path, IEnumerable`1 acceptHeaders)
at Nancy.Routing.DefaultRequestDispatcher.Resolve(NancyContext context)
at Nancy.Routing.DefaultRequestDispatcher.Dispatch(NancyContext context)
at Nancy.NancyEngine.InvokeRequestLifeCycle(NancyContext context, IPipelines pipelines)

nikp commented Oct 10, 2012

@asgerhallas the stacktrace: ComponentNotFoundException

Requested component named 'Castle.Proxies.RootModuleProxy' was not found in the container. Did you forget to register it?
There are 42 other components supporting requested service 'Nancy.NancyModule'. Were you looking for any of them?

at Castle.MicroKernel.DefaultKernel.Castle.MicroKernel.IKernelInternal.Resolve(String key, Type service, IDictionary arguments, IReleasePolicy policy)
at Castle.MicroKernel.DefaultKernel.Resolve(String key, Type service, IDictionary arguments)
at Castle.Windsor.WindsorContainer.Resolve[T](String key)
at Nancy.Bootstrappers.Windsor.WindsorNancyBootstrapper.GetModuleByKey(String moduleKey, NancyContext context)
at Nancy.Routing.DefaultRouteResolver.GetInitializedModuleForMatch(NancyContext context, Tuple4 routeMatchToReturn) at Nancy.Routing.DefaultRouteResolver.CreateRouteAndParametersFromMatch(NancyContext context, Tuple4 routeMatchToReturn)
at Nancy.Routing.DefaultRouteResolver.Resolve(String path, NancyContext context, IRouteCache routeCache)
at Nancy.Routing.DefaultRouteResolver.Resolve(NancyContext context)
at Nancy.Routing.DefaultRequestDispatcher.InvokeRouteResolver(NancyContext context, String path, IEnumerable`1 acceptHeaders)
at Nancy.Routing.DefaultRequestDispatcher.Resolve(NancyContext context)
at Nancy.Routing.DefaultRequestDispatcher.Dispatch(NancyContext context)
at Nancy.NancyEngine.InvokeRequestLifeCycle(NancyContext context, IPipelines pipelines)

@asgerhallas

This comment has been minimized.

Show comment
Hide comment
@asgerhallas

asgerhallas Oct 10, 2012

Contributor

@nikp Thanks.
@thecodejunkie I guess there's something odd with the WindsorModuleKeyGenerator. I'm not experiencing the problem here, but from the stacktrace is seems that the module name has been generated - either with the wrong ModuleKeyGenerator or the proxy was not generated at the time the assembly was scanned. I have not looked into those parts, can you imagine how that could happen?

Contributor

asgerhallas commented Oct 10, 2012

@nikp Thanks.
@thecodejunkie I guess there's something odd with the WindsorModuleKeyGenerator. I'm not experiencing the problem here, but from the stacktrace is seems that the module name has been generated - either with the wrong ModuleKeyGenerator or the proxy was not generated at the time the assembly was scanned. I have not looked into those parts, can you imagine how that could happen?

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie Oct 17, 2012

Member

@asgerhallas No idea. Windsor is not my strong suite =/

Member

thecodejunkie commented Oct 17, 2012

@asgerhallas No idea. Windsor is not my strong suite =/

thecodejunkie added a commit that referenced this pull request Oct 17, 2012

@thecodejunkie thecodejunkie merged commit 3a53b55 into NancyFx:master Oct 17, 2012

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie Oct 17, 2012

Member

I've merged this PR for now so we can include it in the 0.13 release. Thank you

Member

thecodejunkie commented Oct 17, 2012

I've merged this PR for now so we can include it in the 0.13 release. Thank you

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