current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 7 down vote favorite
1

I found out the hardway that access to DbContext in .NET is not threadsafe. I have a singleton for logging things using a dbcontext. The original version uses something like

public Logger{
  private MyContext context;
  private static Logger instance = new Logger();

  private Logger(){
    //init stuff, including context
  }

  public static Logger Instance {
      return this.instance;
  }

  public void someMethod(){
    //do something with this.context
  }
}

I'm thinking of a few solutions:

one would be is not making this a singleton at all, but instantiate a logger each time I need one.

I can think of added overhead as a disadvantage for this way, and simplicity as an advantage.

another one is locking (on the context or type) for each public method: 

public void someMethod(){
  lock(this.context){
    //do something with this.context
  }
}

This adds extra maintenance complexity.

a third one could be one context per thread in a form of

private ConditionalWeakTable<Thread, MyContext> contexts = new ConditionalWeakTable<Thread, MyContext>();

private MyContext Context{
  get {
    return contexts.GetValue(Thread.CurrentThread, createContext());
  }
}

private MyContext createContext(){
  //instantiate a context
}
  • Pro: fairly consice, complexity is isolated
  • Con: batshit insane? Using System.Runtime.CompilerServices for something fairly mondaine, which also isn't what it's meant for.

Am I overlooking any arguments? What would you do?

share|improve this question
    
+1 for the con, although if you are looking at logging as 'mondaine' then why do you need a dbcontext with it? If you are actually logging to a database then in all honesty use a pre-made solution, will save you lots of time and effort. – hyp Sep 18 '12 at 15:05
up vote 5 down vote accepted

General rules of thumb:

  1. In order to take advantage of connection pooling (and you should), database connections should be as short lived as possible. Create, use, then immediately destroy.

  2. Single instance objects should always be agile (defined as not holding system resources e.g. db connections, file handles, etc.).

  3. If you need a single instance object to handle a system resource, it should be done in a protected manner using a named mutex (naming makes a mutex available to all threads across the computer) or Monitor. Within the protected block of code, if the resource is a db connection, apply rule 1.

  4. When using object locking, you should create your locks around an arbitrary object, not the current object's instance e.g.:

    private static readonly object _lockObject = new object();

    ...

    try {
    Monitor.Enter(_lockObject);
    // Do protected stuff...
}
finally {
    Monitor.Exit(_lockObject);
}
share|improve this answer
1  
I disagree with point 4. nothing wrong with locking the object you are using if it is private. – markmnl Dec 3 '13 at 4:56
up vote 2 down vote

I believe that the dbcontext shouldn't be shared between multiple threads - I remember reading about that somewhere, but I can't find the link. Sharing a context between threads can lead to all sorts of problems, though in your case (a logger) that should be limited, cause as I understand it would be used as a 'push only' class.

My suggestion would be to either create the dbcontext each time you use it and wrapp it in a using statement or provide it as a dependency to the logger class and perhaps use an IoC container to control the lifetime of the context. Instantiating a context is not expensive - at least that's what the docs say.

Here are some links that may help:

how to decide on a lifetime for your objectcontext

Why re-initiate the DbContext when using the Entity Framework?

share|improve this answer
    
well, all these ideas were to avoid sharing the context between the threads; that is shouldn't be shared is just the problem I'm trying to fix. – Martijn Sep 18 '12 at 20:12
    
the reason I have it static is to avoid problems with attaching objects from one context to another, which leads to headaches on add vs attach. Having multiple instances of the logging class would fix that though, that is solution #1 I propose above. – Martijn Sep 18 '12 at 20:16
    
I can see the problems that you mention - that's why I'd suggest the IoC. If it's a web app then you can set the lifetime of the context to be per request which is (usually) handled by one thread - unless you're doing some async operations. If it's a win app then depending on the container you choose, you may want to experiment - maybe you'll find a container with a instance per thread lifetime and that should solve the problem. Of course having the context as an external dependency for the logger adds verbosity, but I'd choose that over struggling with the problems you mentioned any time. – Chris Sep 19 '12 at 9:50

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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