Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 5,953 | Comments: 44,409

filter by tags archive

Find the bugAccidental 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?

More posts in "Find the bug" series:

  1. (20 Apr 2011) Why do I get a Null Reference Exception?
  2. (25 Nov 2010) A broken tree
  3. (13 Aug 2010) RavenDB HiLo implementation
  4. (25 Jul 2010) Accidental code reviews

Comments

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

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

greg

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

Shlomo

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

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

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

kostikkv

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

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

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

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

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

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

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

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

Thilak & gunteman,

Precisely!

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

Steve

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

Is equal to:

if(_context == null)

_context = new Acme.Entities.EntitiesObjectContext();

return _context;

Joe Campbell

Which implies a double checked locking issue?

Steve Py

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

Steve Py

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

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

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

Gil
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

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

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

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

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

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

No future posts left, oh my!

RECENT SERIES

  1. The RavenDB Comic Strip (3):
    28 May 2015 - Part III – High availability & sleeping soundly
  2. Special Offer (2):
    27 May 2015 - 29% discount for all our products
  3. RavenDB Sharding (3):
    22 May 2015 - Adding a new shard to an existing cluster, splitting the shard
  4. Challenge (45):
    28 Apr 2015 - What is the meaning of this change?
  5. Interview question (2):
    30 Mar 2015 - fix the index
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats