Ayende @ Rahien

It's a girl

Find the bug: Accidental code reviews

I was working with a client about a problem they had in integrating EF Prof to their application, when my caught the following code base (anonymized, obviously):

public static class ContextHelper
{
     private static Acme.Entities.EntitiesObjectContext _context;

     public static Acme.Entities.EntitiesObjectContext CurrentContext
     {
           get { return _context ?? (_context = new Acme.Entities.EntitiesObjectContext()); }
      }

}

That caused me to stop everything and focus the client’s attentions on the problem that this code can cause.

What were those problems?

Comments

configurator
07/25/2010 03:49 PM by
configurator

Thread safety, first of all.

Second, there are many points against singletons which I'm too lazy to iterate, especially since there are cases where singletons are exactly the right thing.

What's wrong with

public static class ContextHelper {

public static readonly EntitiesObjectContext CurrentContext = new EntitiesObjectContext();

} ?

alberto
07/25/2010 03:56 PM by
alberto

Could it be a race condition where more than one context gets created?

greg
07/25/2010 04:13 PM by
greg

Does the 2nd half of the ?? Operator even return the correct type?

Shlomo
07/25/2010 04:31 PM by
Shlomo

There's a missing closing curly brace, though I assume that's a typo

Thilak Nathen
07/25/2010 05:24 PM by
Thilak Nathen

Object contexts are reasonably volatile resources... like NH sessions. Not a good idea to singleton them. They are meant for short lived usages.

Wile E. Coyote
07/25/2010 06:26 PM by
Wile E. Coyote

this is why I never catch that @#$@ bird

kostikkv
07/25/2010 07:24 PM by
kostikkv

It’s not even a singleton. This code shows that EntitiesObjectContext may have internal/ public constructor.

evereq
07/25/2010 07:37 PM by
evereq

Nice :)

a) My # Re to 'configurator':

With code like

public static class ContextHelper {

   public static readonly EntitiesObjectContext CurrentContext = new  EntitiesObjectContext();

}

nothing wrong, except that CurrentContext created when app starts instead of "lazy" way from original code with C# "getter"... While sure it's sometimes maybe make sense to create objects right after App start, usually better to put such logic into bootstrappers.. and I never see that contexts (entities contexts) somebody create such a way, i.e. ones after app starts :D and keep forever... but sure as always "it depends" :)

b) with original code from famous post author issue that I see, like in most of real world situations, is that programmer probably want CurrentContext to be actually "current", not the one that was created when you first time use CurrentContext in blog post code... :)

c) Sure no thread safety at all.. 100% agree! but I was think that it's normal to see "warn" sign when you see static fields and no "lock" in the moment when you fill them :)

OmariO
07/25/2010 07:46 PM by
OmariO

Thread unsafe work with shared variable . In such situation it's possible that concurrent clients get diffrent instances of the context class.

kostikkv
07/25/2010 08:08 PM by
kostikkv

Yeah, probably EntitiesObjectContext:System.Data.Objects.ObjectContext and they decided to reuse its instance, but there was some threading problem, ‘cause the app was an ASP.NET app or something.

Mistertom
07/25/2010 09:07 PM by
Mistertom

nothing wrong, except that CurrentContext created when app starts

No, the initialization is lazy : the static constructor is called when a reference to the class is found by the JIT.

gunteman
07/25/2010 09:57 PM by
gunteman

The ObjectContext is a Unit of Work, and it's completely bonkers to make it static. Any thread safety or race condition issues are irrelevant in comparison

Ayende Rahien
07/25/2010 11:31 PM by
Ayende Rahien

Configurator,

Thread Safety is a problem, sure.

But let us assume that we are talking about single threaded code, it is still wrong, why?

Ayende Rahien
07/25/2010 11:32 PM by
Ayende Rahien

Alberto,

You are trying to find a bug in the dark, when it is sitting under the lamp and chuckling.

In other words, don't try to find non obvious / complex ones, this one has glaring issues.

Ayende Rahien
07/25/2010 11:35 PM by
Ayende Rahien

Thilak & gunteman,

Precisely!

Steve Py
07/25/2010 11:36 PM by
Steve Py

As mentioned above, If their context implementation is an object context then it should not be treated as a Singleton instance.

Still I'm pretty sure that modified code doesn't work as they intended (aside from the missing brace) It's probably supposed to be something like: [Namespaces stripped out for clarity]

public static EntitiesObjectContext CurrentContext

{

get 

{ 

    _context = _context ?? new EntitiesObjectContext();

    return _context;

 }

}

Additional little unpleasant smells: (Not context specific) Returning concrete types. Locks implementation details in. I'd define an interface for the context and have a factory create a specific concrete instance. Better yet I'd configure an IOC container to resolve it as a dependency of whatever class needed to avoid hand-writing factories.

Ayende Rahien
07/25/2010 11:38 PM by
Ayende Rahien

Steve

return context ?? (context = new Acme.Entities.EntitiesObjectContext())

Is equal to:

if(_context == null)

_context = new Acme.Entities.EntitiesObjectContext();

return _context;

Joe Campbell
07/26/2010 02:08 AM by
Joe Campbell

Which implies a double checked locking issue?

Steve Py
07/26/2010 02:53 AM by
Steve Py

So it is.... That just looks so wrong. :)

Steve Py
07/26/2010 03:12 AM by
Steve Py

I certainly hope this wasn't an ASP.Net application.

Mike Minutillo
07/26/2010 03:17 AM by
Mike Minutillo

With EF4 the context performs change tracking by keeping a reference to each object it produces so over time you'll end up with every entity in your database in memory.

Potentially unrelated parts of the app can mess with each other by calling SaveChanges() at inappropriate (unexpected) times persisting changes which might be incomplete. Transactions would be an absolute nightmare.

Kelly Stuard
07/26/2010 05:32 AM by
Kelly Stuard

Side note: It's odd that I don't see anything related to the newly-released NHibernate 3.0.0Alpha1.

Gil
07/26/2010 06:48 AM by
Gil

With a static context we will get a memory leak since the context is acting as a cache for entities that were returned from the database. Also, it creates a state entry for each entity and additional objects to track entity relationships which of course in the long run will fill the memeory and crush the application.

Giuliano
07/26/2010 01:21 PM by
Giuliano

Ok, All context objects will stay in the cache after the first access, after some time we'll have all database objects in cache(objectcontexts objects). Am I right? I just want to know the right way to do this with objectcontext. On the L2Sql i putted the DataContext in HttpContext.item, it was hardly stressed, more then 5000 updates, inserts per second, 10 users sim and never lost a transaction. Now i wanna port linq 2 sql to EF. Wich one will be the right way or better way?

Gian Maria Ricci
07/26/2010 04:00 PM by
Gian Maria Ricci

It will use the same EntityObjectContext for the whole application, this can cause problem if multiple thread will access the context. But the error is because this is like using the same session in all the program, what happens if during a flush we have an exception? After an exception in a flush / update we need to change the context, because it is no more usable.

alk.

Scott
07/27/2010 12:55 PM by
Scott

Potential Problems:

  1. Bad implementation of a singleton, if indeed that was the intention.

  2. Has potential to cause problems (data integrity and possible race condition) if the same session uses multiple threads.

  3. Static implementation will stay in scope and not garbage collect until the thread shuts down.

  4. Readonly property will not allow a graceful recovery if an exception happens.

Alois Kraus
07/28/2010 05:55 AM by
Alois Kraus

I did not much tinker with databases but I would ask the question why the CurrentContext has no set property?

If I ever choose to open another DB I would like to use a new context. Since it is a static class I think that there is currently no way to inject a fresh context to the underlying object chain.

Just my non educated guess.

Yours,

Alois Kraus
Comments have been closed on this topic.