Skip to content

Loading…

Fix thread race conditions when automatically registering components with Castle Windsor #16

Closed
wants to merge 3 commits into from

2 participants

@MattFenelon

Hi Seb,

I'm hoping this is a fix for the race conditions mentioned in this thread on the OR mailing list. We've also seen the issue crop up when registering codecs.

I'd be interested to hear your thoughts on whether there would be any problems registering this way?

@MattFenelon MattFenelon For some componennts OR will register the components with the IoC con…
…tainer just-in-time. This can cause race conditions if many requests are attempting to register the component at the same time. Added locking when adding dependencies to Windsor's container to fix this.
b2f5408
@serialseb
openrasta member

This would assume that we are happy to have the IDependencyResolver be designed as thread-safe, which means changing all the other container implementations. If it's a castle-specific problem where the methods on IWindsorContainer is not thread safe, then that patch can be fine. Can someone check with them?

The issue that's higlighted in that thread cannot happen for codecs, those are registered upon startup, once only and the configuration itself is thread safe. If you're seeing codec-related issues, it's not the same problem.

@MattFenelon

It does look like we are suffering the same problem, with our codecs, as spoken about in the original thread. In the stack trace below, the exception originates in ResponseEntityWriterContributor.WriteResponse(ICommunicationContext context), when it is calling DependencyManager.GetService. The only usage I can find of GetService, inside that method, is when the ResponseCodec.CodecType is resolved. The resolution of the codec fails and falls through to the auto registration process.

System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
Parameter name: capacity
   at System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument)
   at System.Collections.Generic.Dictionary`2..ctor(Int32 capacity, IEqualityComparer`1 comparer)
   at System.Collections.Generic.Dictionary`2..ctor(IDictionary`2 dictionary, IEqualityComparer`1 comparer)
   at System.Collections.Generic.Dictionary`2..ctor(IDictionary`2 dictionary)
   at Castle.MicroKernel.Handlers.AbstractHandler.DependencySatisfied(Boolean& stateChanged)
   at Castle.MicroKernel.DefaultKernel.DoActualRaisingOfHandlersChanged()
   at Castle.MicroKernel.DefaultKernel.RaiseHandlersChanged()
   at Castle.MicroKernel.DefaultKernel.RegisterHandler(String key, IHandler handler, Boolean skipRegistration)
   at Castle.MicroKernel.DefaultKernel.RegisterHandler(String key, IHandler handler)
   at Castle.MicroKernel.DefaultKernel.AddComponent(String key, Type serviceType, Type classType, LifestyleType lifestyle, Boolean overwriteLifestyle)
   at Castle.Windsor.WindsorContainer.AddComponentLifeStyle(String key, Type serviceType, Type classType, LifestyleType lifestyle)
   at OpenRasta.DI.Windsor.WindsorDependencyResolver.AddDependencyCore(Type dependent, Type concrete, DependencyLifetime lifetime)
   at OpenRasta.DI.Windsor.WindsorDependencyResolver.AddDependencyCore(Type handlerType, DependencyLifetime lifetime)
   at OpenRasta.DI.DependencyResolverCore.AddDependency(Type concreteType, DependencyLifetime lifetime)
   at OpenRasta.DI.DependencyManager.GetService(Type dependencyType)
   at OpenRasta.Pipeline.Contributors.ResponseEntityWriterContributor.WriteResponse(ICommunicationContext context)
   at OpenRasta.Pipeline.PipelineRunner.ExecuteContributor(ICommunicationContext context, ContributorCall call)

I think the reason we, specifically, are seeing this problem, is due to our use of custom codecs, which are registered using the fluent interface. Searching through the OR code I found that the default codecs are registered, with the IoC container, at application start up. I've been unable to find any code that registers codecs configured using the FI, with the IoC container, other than the just-in-time statements, as used in ResponseEntityWriterContributor above. Does this seem right, or have I missed something?

@serialseb
openrasta member
@MattFenelon

I second that. When the auto-registration problem bit us again, this time with the codecs (handlers got us first), I wanted to put something more permanent in place than registering these components, manually, at app startup. Hence the locking. Removing auto-reg completely would put my mind at rest :)

It does sound like a major change though? It'd probably be worth while to put this small patch in place, in the meantime, to stop others suffering from the same issue.

@MattFenelon

Seb, in case you're interested I've started a thread about this problem on the Castle mailing list:
http://groups.google.com/group/castle-project-users/browse_thread/thread/6f6edd7718c411c

@MattFenelon MattFenelon _windsorContainer.AddComponentLifeStyle() is not thread safe, but Reg…
…ister() is.

Removed changes for 1.0 as I have no way of testing them.
Adding locking to AddDependencyInstanceCore as it can cause the container to get into an invalid state, if many threads are registering at the same time.
The lock object is now static to ensure it will apply across all instances of the WindsorDependencyResolver class.
9195c0e
@MattFenelon

I've updated the pull request because even after applying the original patch we continued to see similar issues with registration.

While investigating I found that the AddComponentLifeStyle methods on WindsorContainer are not thread-safe, nor are the methods on IKernel for registration. I've swaped those methods for their thread safe equivalents, Register(Component.For...) and removed the lock I put in place. I've also added more locking around the other routes into component registration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 24, 2011
  1. @MattFenelon

    For some componennts OR will register the components with the IoC con…

    MattFenelon committed
    …tainer just-in-time. This can cause race conditions if many requests are attempting to register the component at the same time. Added locking when adding dependencies to Windsor's container to fix this.
Commits on Jun 30, 2011
  1. @MattFenelon

    _windsorContainer.AddComponentLifeStyle() is not thread safe, but Reg…

    MattFenelon committed
    …ister() is.
    
    Removed changes for 1.0 as I have no way of testing them.
    Adding locking to AddDependencyInstanceCore as it can cause the container to get into an invalid state, if many threads are registering at the same time.
    The lock object is now static to ensure it will apply across all instances of the WindsorDependencyResolver class.
Commits on Aug 9, 2011
  1. @MattFenelon
Showing with 50 additions and 29 deletions.
  1. +50 −29 src/castle/OpenRasta.DI.Windsor/WindsorDependencyResolver.cs
View
79 src/castle/OpenRasta.DI.Windsor/WindsorDependencyResolver.cs
@@ -28,6 +28,7 @@ namespace OpenRasta.DI.Windsor
public class WindsorDependencyResolver : DependencyResolverCore, IDependencyResolver
{
readonly IWindsorContainer _windsorContainer;
+ static readonly object _containerLock = new object();
public WindsorDependencyResolver(IWindsorContainer container)
{
@@ -83,26 +84,37 @@ protected override IEnumerable<TService> ResolveAllCore<TService>()
protected override void AddDependencyCore(Type dependent, Type concrete, DependencyLifetime lifetime)
{
string componentName = Guid.NewGuid().ToString();
- if (lifetime != DependencyLifetime.PerRequest)
+ lock (_containerLock)
{
+ if (lifetime != DependencyLifetime.PerRequest)
+ {
#if CASTLE_20
- _windsorContainer.AddComponentLifeStyle(componentName, dependent, concrete,
- ConvertLifestyles.ToLifestyleType(lifetime));
+ _windsorContainer.Register(
+ Component
+ .For(dependent)
+ .ImplementedBy(concrete)
+ .Named(componentName)
+ .LifeStyle.Is(ConvertLifestyles.ToLifestyleType(lifetime)));
#elif CASTLE_10
- _windsorContainer.AddComponentWithLifestyle(componentName, dependent, concrete, ConvertLifestyles.ToLifestyleType(lifetime));
+ _windsorContainer.AddComponentWithLifestyle(componentName, dependent, concrete, ConvertLifestyles.ToLifestyleType(lifetime));
#endif
- }
- else
- {
+ }
+ else
+ {
#if CASTLE_20
- _windsorContainer.Register(
- Component.For(dependent).Named(componentName).ImplementedBy(concrete).LifeStyle.Custom(typeof (ContextStoreLifetime)));
+ _windsorContainer.Register(
+ Component
+ .For(dependent)
+ .Named(componentName)
+ .ImplementedBy(concrete)
+ .LifeStyle.Custom(typeof(ContextStoreLifetime)));
#elif CASTLE_10
- ComponentModel component = _windsorContainer.Kernel.ComponentModelBuilder.BuildModel(componentName, dependent, concrete, null);
- component.LifestyleType = ConvertLifestyles.ToLifestyleType(lifetime);
- component.CustomLifestyle = typeof (ContextStoreLifetime);
- _windsorContainer.Kernel.AddCustomComponent(component);
+ ComponentModel component = _windsorContainer.Kernel.ComponentModelBuilder.BuildModel(componentName, dependent, concrete, null);
+ component.LifestyleType = ConvertLifestyles.ToLifestyleType(lifetime);
+ component.CustomLifestyle = typeof(ContextStoreLifetime);
+ _windsorContainer.Kernel.AddCustomComponent(component);
#endif
+ }
}
}
@@ -111,9 +123,28 @@ protected override void AddDependencyInstanceCore(Type serviceType, object insta
string key = Guid.NewGuid().ToString();
if (lifetime == DependencyLifetime.PerRequest)
{
- // try to see if we have a registration already
var store = (IContextStore) Resolve(typeof (IContextStore));
- if (_windsorContainer.Kernel.HasComponent(serviceType))
+ // try to see if we have a registration already
+ if (_windsorContainer.Kernel.HasComponent(serviceType) == false)
+ {
+ lock (_containerLock)
+ {
+ if (_windsorContainer.Kernel.HasComponent(serviceType) == false)
+ {
+ var component = new ComponentModel(key, serviceType, instance.GetType());
+ var customLifestyle = typeof(ContextStoreLifetime);
+ component.LifestyleType = LifestyleType.Custom;
+ component.CustomLifestyle = customLifestyle;
+ component.CustomComponentActivator = typeof(ContextStoreInstanceActivator);
+ component.ExtendedProperties[Constants.REG_IS_INSTANCE_KEY] = true;
+ component.Name = component.Name;
+
+ _windsorContainer.Kernel.AddCustomComponent(component);
+ store[component.Name] = instance;
+ }
+ }
+ }
+ else
{
var handler = _windsorContainer.Kernel.GetHandler(serviceType);
if (handler.ComponentModel.ExtendedProperties[Constants.REG_IS_INSTANCE_KEY] != null)
@@ -126,23 +157,13 @@ protected override void AddDependencyInstanceCore(Type serviceType, object insta
throw new DependencyResolutionException("Cannot register an instance for a type already registered");
}
}
- else
- {
- var component = new ComponentModel(key, serviceType, instance.GetType());
- var customLifestyle = typeof (ContextStoreLifetime);
- component.LifestyleType = LifestyleType.Custom;
- component.CustomLifestyle = customLifestyle;
- component.CustomComponentActivator = typeof (ContextStoreInstanceActivator);
- component.ExtendedProperties[Constants.REG_IS_INSTANCE_KEY] = true;
- component.Name = component.Name;
-
- _windsorContainer.Kernel.AddCustomComponent(component);
- store[component.Name] = instance;
- }
}
else if (lifetime == DependencyLifetime.Singleton)
{
- _windsorContainer.Kernel.AddComponentInstance(key, serviceType, instance);
+ lock (_containerLock)
+ {
+ _windsorContainer.Kernel.AddComponentInstance(key, serviceType, instance);
+ }
}
}
Something went wrong with that request. Please try again.