Adding action id to a history repository

Question

In my system I have a history repository. My history class is big and with a lot of information regarding operations. My warehouse needs, for some actions to know which histories are caused by the same action.

My first idea was to create an overloaded version of each method requiring to write histories and make all the clients using that services create and supply a guid if they want to specify an action.

As my system has a lot of services that some use other services and a lot of clients that may need to specify an action that solution sounds bad. Complicated, error prone and would pollute code that is not interested in knowing anything about this.

My solution: I created a ActionIdContext class that my clients can use to specify a group action and then my HistoryBuilder uses a static property to get the current ActionId if any. This way no services or overloads have to be created or changed.

Do you like this design or maybe you have another idea?

public class ActionIdContext : IDisposable
{
 private static Guid? actionId;
 private readonly bool isActionIdSet;
 public ActionIdContext()
 {
 if (actionId == null)
 {
 actionId = Guid.NewGuid();
 isActionIdSet = true;
 }
 }
 public static Guid? ActionId
 {
 get { return actionId; }
 set { actionId = value; }
 }
 public void Dispose()
 {
 if (isActionIdSet)
 {
 actionId = null;
 }
 }
}
public class HistoryBuilder
{
 private string text;
 public HistoryBuilder SetText(string value)
 {
 this.text = value;
 return this;
 }
 public History Build()
 {
 return new History()
 {
 Text = text,
 ActionId = ActionIdContext.ActionId
 };
 }
}
public class Client
{
 private readonly ISomeService someService;
 public Client(ISomeService someService)
 {
 this.someService = someService;
 }
 public void DoSomeAction()
 {
 someService.DoSomething("Flying in the sky");
 }
 public void DoSomeGroupAction()
 {
 using (new ActionIdContext())
 {
 someService.DoSomething("Driving in the corner");
 someService.DoSomething("Reading Hulk!");
 someService.DoSomething("Doing math");
 }
 }
}

• RobH: Can action id contexts be shared between threads?
• Jakob: @RobH Yes it is possible, should maybe add ThreadStatic for the static variable or ReaderWriterLockSlim, wanted first to know if you like the idea of this context class to solve a problem like this :)
• RobH: I've rolled back your edit, see here for what you can and cannot do when you receive an answer.

Answer 1

The problem is that the action id is static so it will be shared with all instances of the ActionIdContext class. That means that 2 separate pieces of work excuting at the same time could log everything with the same action id.

var client1 = new Client(someService);
var client2 = new Client(someService);
var client3 = new Client(someService);
Task.Run(() => client1.DoSomeGroupAction());
Task.Run(() => client2.DoSomeGroupAction());
Task.Run(() => client3.DoSomeGroupAction());

You have no idea how this code will log the actions - it could log them all with the same action id, it could log some with no action id and some with a common action id. It could also log some with one action id and others with another.

The point I'm trying to make is, although a nice idea in theory, you can't locally instantiate state then share it via a static property and expect it to work.

Can't you add a LogHistories(Guid actionId, param History[] histories) method or something?

---

You should implement the full IDisposable.Dispose pattern;

---

public static Guid? ActionId
{
 get { return actionId; }
 set { actionId = value; }
}

would be better as an auto property.

---

HistoryBuilder is a bit weird - Builders are generally used with immutables or objects with very complex constructors (in my experience).

• Jakob: Thank you for your answer. I have added a ThreadStatic attribute that solves the threading problem. I also implemented IDisposable correctly. HistoryBuilder is a demo class, my real implementation is more complex, I didn't want to focus the question on that class only if this is a good idea. I have a lot of services that are adding histories, if would crate a "LogMany" method then I would have to change all my services and that is what I am trying to avoid like described in my question.
• RobH: [ThreadStatic] is insufficient to deal with threading issues if there is a thread pool involved - is this used in a web app?
• Jakob: at the moment no, in the future, yes maybe. I could then change ActionIdContext to have a context per request if called from within a web application and else a context per thread. How does that sound?

Answer 2

I have thought about this and viewed similar solutions and think this is a idea that can work.

The main problem is that it is not thread-safe. I made some improvements to solve this problem for a application and a website. I made some other minor modifications.

---

Here are my changes:

1. I renamed ActionIdContext to ActionGroupContext and the static public property Id instead of ActionId.
2. I simplified the logic in ActionGroupContext maintaining the Guid.
3. I correctly implemented IDisposable
4. I created a GuidStore to maintain the current CallContext and detects if it should be running a HttpStore or ThreadStore.

---

public class ActionGroupContext : IDisposable
{
 private static readonly GuidStore GuidStore = new GuidStore("Action-Group-Context-Store-Key");
 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 interface IStore
{
 object GetData();
 void SetData(object data);
}
public class GuidStore
{
 private readonly IStore store;
 public GuidStore(string storeName)
 {
 if (HttpStore.IsContextAvailable())
 {
 store = new HttpStore(storeName);
 }
 else
 {
 store = new ThreadStore(storeName);
 }
 }
 public Guid GetGuid()
 {
 return (Guid)store.GetData();
 }
 public void SetGuid(Guid? guid)
 {
 store.SetData(guid);
 }
}
public class HttpStore : IStore
{
 private readonly string storeKey;
 public HttpStore(string storeKey)
 {
 if (!IsContextAvailable())
 {
 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 IsContextAvailable()
 {
 return HttpContext.Current != null;
 }
}
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);
 }
}