Monday, November 2, 2015

String trimming WCF message inspector for XML binding

How often your users copy-paste data from somewhere to your web app and then post something that is incorrect because some extra characters are copied from the source text? Is user name really “ John ” “ Smith ”?

Sooner or later at the server side, the input strings are validated but often what you do first is a simple trimming so that “ John” becomes “John”. The question is then not “to trim or not to trim” but rather “when the trimming should occur to be as ellegant as possible”.

String trimming on HTTP input is a convenient solution. The data that is processed by an MVC controller, a WebApi controller or a WCF service code is already trimmed so that you don’t have to put any extra effort on trimming at the business level of your code. This approach is common and solutions for MVC or WebAPI can easily be found.

My specific problem was that I needed the same for WCF and unfortunately, I couldn’t find anything ready to use. My previous experience with WCF message inspectors suggested that a smart inspector is a way to go. A series of blog entries by Carlos Figueria is a valuable source of information on that topic, however this specific requirement of a trimming inspector has no previous solutions (or I couldn’t find any).

Without any further ado, let’s see how such message inspector is implemented:

/// <summary>
/// WCF inspektor trimujący stringi
/// </summary>
public class StringTrimmingMessageInspector : IDispatchMessageInspector
{
    #region IDispatchMessageInspector Members
 
    public object AfterReceiveRequest(
        ref System.ServiceModel.Channels.Message request,
        IClientChannel channel, InstanceContext instanceContext )
    {
        if ( !request.IsEmpty )
        {
            MessageBuffer buffer = request.CreateBufferedCopy( Int32.MaxValue );
            request = buffer.CreateMessage();
 
            XmlDocument xmlDoc = new XmlDocument();
 
            using ( MemoryStream ms = new MemoryStream() )
            {
                XmlWriter writer = XmlWriter.Create( ms );
                request.WriteMessage( writer ); // the message was consumed here
                writer.Flush();
                ms.Position = 0;
                xmlDoc.Load( ms );
            }
 
            XmlNamespaceManager manager = new XmlNamespaceManager( xmlDoc.NameTable );
            manager.AddNamespace( "s", "http://schemas.xmlsoap.org/soap/envelope/" );
 
            XmlNode bodyNode = xmlDoc.SelectSingleNode( "//s:Envelope/s:Body", manager );
            this.ChangeNode( bodyNode );
 
            Message newMessage = Message.CreateMessage( request.Version, request.Headers.Action, bodyNode.FirstChild );
            newMessage.Properties.CopyProperties( request.Properties );
 
            request = newMessage;
        }
 
        return null;
    }
 
    private void ChangeMessage( XmlDocument doc )
    {
        if ( doc == null ) return;
 
        ChangeNode( doc.DocumentElement );
    }
 
    private void ChangeNode( XmlNode node )
    {
        if ( node == null ) return;
        if ( node.NodeType == XmlNodeType.Text ) node.InnerText = node.InnerText.Trim();
 
        foreach ( XmlNode childNode in node.ChildNodes )
            ChangeNode( childNode );
    }
 
    public void BeforeSendReply( ref System.ServiceModel.Channels.Message reply, object correlationState )
    {
    }
 
    #endregion
}

The inspector assumes the message is always an XML document and this is true for XML based bindings. A request is always a SOAP envelope that has headers and a body. Inside the inspector I retrieve the body node of an incoming message and I run a simple recursive function on the node that walks over the tree of XML nodes and whenever it find a node of text type, it trims its contents. I then recreate the message using traversed node as the body.

To apply the inspector, I also need a simple endpoint behavior

/// <summary>
/// Podstawowa infrastruktura rozszerzania
/// </summary>
public class StringTrimmingBehavior : IEndpointBehavior
{
    #region IEndpointBehavior Members
 
    public void AddBindingParameters( ServiceEndpoint endpoint, 
       System.ServiceModel.Channels.BindingParameterCollection bindingParameters )
    {
    }
 
    public void ApplyClientBehavior( ServiceEndpoint endpoint, 
       System.ServiceModel.Dispatcher.ClientRuntime clientRuntime )
    {
    }
 
    public void ApplyDispatchBehavior( ServiceEndpoint endpoint, 
       System.ServiceModel.Dispatcher.EndpointDispatcher endpointDispatcher )
    {
        endpointDispatcher.DispatchRuntime.MessageInspectors
           .Add( new StringTrimmingMessageInspector() );
    }
 
    public void Validate( ServiceEndpoint endpoint )
    {
    }
 
    #endregion
}
And this is it, please feel free to use/modify this for your own purposes.

Friday, October 16, 2015

Short story of an ASP.NET app memory leak

One of our apps was leaking memory. This passed unnoticed for few years but as the app is used by more and more users, the leak became problematic. The app pool memory had been growing to few gigabytes until the pool was recycled every 24 hours.

The first thing to do in such case is to determine if the leak is caused by a managed code or rather an unmanaged code. This can be done with performance counters. Then, the DebugDiag can be used to collect a memory dump and then the dump can be analyzed to find what objects occupy the memory.

In our case it turned out the Unity container was the culprit or rather, an unfortunate implementation of a unity controller factory for ASP.NET MVC:

public class UnityControllerFactory :
    IControllerFactory
{
    #region IControllerFactory
 
    private IUnityContainer _container;
    private IControllerFactory _innerFactory;
 
    public UnityControllerFactory( IUnityContainer container )
        : this( container, new DefaultControllerFactory() )
    {
    }
 
    protected UnityControllerFactory( IUnityContainer container,
                                     IControllerFactory innerFactory )
    {
        _container = container;
        _innerFactory = innerFactory;
    }
 
    public IController CreateController( RequestContext requestContext,
                                        string controllerName )
    {
        if ( _container.IsRegistered<IController>( controllerName.ToLowerInvariant() ) )
            return _container.Resolve<IController>( controllerName.ToLowerInvariant() );
 
        var controller = _innerFactory.CreateController( requestContext, controllerName );
 
        if ( controller is IBuildUpController )
        {
            var controllerContainer = _container.CreateChildContainer() )
            
            ( (IBuildUpController)controller ).PreBuildUp( controllerContainer );
            controllerContainer.BuildUp( controller.GetType(), controller );
            ( (IBuildUpController)controller ).PostBuildUp( controllerContainer );
        }
 
        return controller;
    }
 
    public void ReleaseController( IController controller )
    {
        if ( controller is IDisposable )
            ( (IDisposable)controller ).Dispose();
 
        _container.Teardown( controller );
    }
 
    public System.Web.SessionState.SessionStateBehavior GetControllerSessionBehavior( 
       RequestContext requestContext, 
       string controllerName )
    {
        return System.Web.SessionState.SessionStateBehavior.Default;
    }
 
    #endregion
 
}

There is an idea behind such factory – it tries to be smart.

If a controller type is registered, it uses the container to resolve an instance. This is to allow constructor injection.

If a controller type is not registered, it still allows property injection assuming the type is decorated with an attribute.

Since this is a multitenant app, the factory uses a child container to be able to configure services differently for different tenants (obviously, looking at the code, the idea of conditional registrations depending on the tenant was never implemented).

Unfortunately also, this is not obvious why this code lacks the memory.

The problem is the way Unity creates a child container. By inspecting the source code I can learn that the CreateChildContainer method actually calls new UnityContainer( this ) which is implemented as

private UnityContainer(UnityContainer parent)
{
    this.parent = parent;
 
    if (parent != null)
    {
        parent.lifetimeContainer.Add(this); // <- culprit!!!
    }
 
    ...
}

The obvious thing here is that the child registers itself in the lifetime container of the parent which means that consecutive children will make the list grow and grow. And of course there is a way to “unregister”:

protected virtual void Dispose(bool disposing)
{
    if (disposing)
    {
        if (lifetimeContainer != null)
        {
            lifetimeContainer.Dispose();
            lifetimeContainer = null;
 
            if (parent != null && parent.lifetimeContainer != null)
            {
                parent.lifetimeContainer.Remove(this); // <- ha!
            }
        }
 
        extensions.OfType<IDisposable>().ForEach(ex => ex.Dispose());
        extensions.Clear();
    }
}

which in turn leads to an obvious fix to the original unity factory

public IController CreateController( RequestContext requestContext,
                                    string controllerName )
{
    if ( _container.IsRegistered<IController>( controllerName.ToLowerInvariant() ) )
        return _container.Resolve<IController>( controllerName.ToLowerInvariant() );
 
    var controller = _innerFactory.CreateController( requestContext, controllerName );
 
    if ( controller is IBuildUpController )
    {
        using ( var controllerContainer = _container.CreateChildContainer() )
        {
            ( (IBuildUpController)controller ).PreBuildUp( controllerContainer );
            controllerContainer.BuildUp( controller.GetType(), controller );
            ( (IBuildUpController)controller ).PostBuildUp( controllerContainer );
        } // <- this calls Dispose and unregisters the child from the parent
    }
 
    return controller;
}

An important issue that potentially occurs here is what happens in a concurrent environment where controllers are created upon user requests so that child containers are constantly added and removed from the parent container list. And of course, List<T> methods for adding/removing items are not thread safe.

It turns out fortunately, that Unity doesn’t use just a List<T> for the lifetimeContainer list, rather it has a custom implementation of a synchronized list that locks adding and removing making it thread safe.

Lesson learned, leak fixed, warning dismissed.