3
\$\begingroup\$

My services(or services that depend on services) need to know about a current action group id. To add an extra optional parameter to each service for the id was an idea but in my current environment to difficult and ugly because every service would have to take in the parameter only to pass it to other services.

My solution was a class ActionGroupContext which stores the current context in a static ThreadStore or HttpStore.

What I don't like especially with my solution is that even though you are not do web application you have to include a reference to System.Web, but the convenience is great, the user does not need any setup code.

Do you have any improvement ideas?

Demo usage:

public void SomeApplicationMehtod()
{
    using (new ActionGroupContext())
    {
        someService.SomeMehtod();
    }
}

public class SomeService
{
    public void SomeMehtod()
    {
        var id = ActionGroupContext.Id;

        //Do something with the Id.

    }
}

The code:

public class ActionGroupContext : IDisposable
{
    private static readonly GuidStore GuidStore = new GuidStore("Action-Group-Id-Store");

    private readonly Guid? oldId;
    public ActionGroupContext()
    {
        oldId = GuidStore.GetGuid();
        GuidStore.SetGuid(Id ?? Guid.NewGuid());
    }

    public static Guid? Id
    {
        get { return GuidStore.GetGuid(); }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            GuidStore.SetGuid(oldId);
        }
    }
}

public class ThreadStore : IStore
{
    private readonly LocalDataStoreSlot slot;
    public ThreadStore(string storeKey)
    {
        slot = Thread.GetNamedDataSlot(storeKey);
    }

    public object GetData()
    {
        return Thread.GetData(slot);
    }

    public void SetData(object data)
    {
        Thread.SetData(slot, data);
    }
}

public interface IStore
{
    object GetData();
    void SetData(object data);
}

public class ThreadStore : IStore
{
    private readonly LocalDataStoreSlot slot;
    public ThreadStore(string storeKey)
    {
        slot = Thread.GetNamedDataSlot(storeKey);
    }

    public object GetData()
    {
        return Thread.GetData(slot);
    }

    public void SetData(object data)
    {
        Thread.SetData(slot, data);
    }
}

public class HttpStore : IStore
{
    private readonly string storeKey;

    public HttpStore(string storeKey)
    {
        if (!IsContextForStoreAvailable())
        {
            throw new InvalidOperationException("HttpContext is not available");
        }
        this.storeKey = storeKey;
    }

    public object GetData()
    {
        return HttpContext.Current.Items(storeKey);
    }

    public void SetData(object data)
    {
        HttpContext.Current.Items(storeKey) = data;
    }

    public static bool IsContextForStoreAvailable()
    {
        return HttpContext.Current != null;
    }
}
\$\endgroup\$
3
\$\begingroup\$

Your Dispose pattern is somehow flawed.

public void Dispose()
{
    Dispose(true);
    GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
    if (disposing)
    {
        GuidStore.SetGuid(oldId);
    }
}  

You don't check if the object already is disposed, so calling Dispose() over and over will result in each time calling GuidStore.SetGuid(oldId);.

So using a private variable for saving if the object is disposed would be the way to go.

private bool disposed = false;

public void Dispose()
{
    Dispose(true);
    GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
    if (!disposed && disposing)
    {
        GuidStore.SetGuid(oldId);
        disposed = true;
    }
}

Also for completeness you should also use the finalizer like so

~ActionGroupContext()
{
    Dispose(false);
}  

One thing what should never happen is that a call to Dispose() throws an exception. If you are 100% sure this can't happen, you are fine. If you aren't 100% sure, you should enclose the call to SetGuid() inside a try..catch block.

Please read this: proper-use-of-the-idisposable-interface

In your posted code I see neither in the ThreadStore nor in the HttpStore any usage of this ActionGroupContext so I see no reason to reference System.Web to use this context.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your reply. This code is all in the same assembly and ActionGroupContext creates a new instance of GuidStore which uses HttpStore which references System.Web. You see nothing wrong with that? I have this in the same assembly as my core, which has nothing to do in particular with the web... Would you move it and maybe force the user(programmer) to install either a HttpStore or ThreadStore? \$\endgroup\$ – Jakob Aug 10 '15 at 15:12
  • \$\begingroup\$ If you only use the HttpContext to store, but you don't do a web application, you should consider to use a custom class which exposes an ConcurrentDictionary to serve the same purpose like HttpContext.Current.Items. Concurrent because it is thread safe. \$\endgroup\$ – Heslacher Aug 10 '15 at 15:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.