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:
- (29 Feb 2016) When you can't rely on your own identity
- (05 Jan 2016) The case of the degrading system–Answer
- (04 Jan 2016) The case of the degrading system
- (11 Sep 2015) The concurrent memory buster
- (20 Apr 2011) Why do I get a Null Reference Exception?
- (25 Nov 2010) A broken tree
- (13 Aug 2010) RavenDB HiLo implementation
- (25 Jul 2010) Accidental code reviews
Comments
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 {
} ?
Could it be a race condition where more than one context gets created?
Does the 2nd half of the ?? Operator even return the correct type?
There's a missing closing curly brace, though I assume that's a typo
Object contexts are reasonably volatile resources... like NH sessions. Not a good idea to singleton them. They are meant for short lived usages.
this is why I never catch that @#$@ bird
It’s not even a singleton. This code shows that EntitiesObjectContext may have internal/ public constructor.
Nice :)
a) My # Re to 'configurator':
With code like
public static class ContextHelper {
}
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 :)
Thread unsafe work with shared variable . In such situation it's possible that concurrent clients get diffrent instances of the context class.
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.
No, the initialization is lazy : the static constructor is called when a reference to the class is found by the JIT.
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
Configurator,
Thread Safety is a problem, sure.
But let us assume that we are talking about single threaded code, it is still wrong, why?
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.
Greg,
Yes, it does.
Shlomo,
Fixed, thanks
Thilak & gunteman,
Precisely!
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
{
}
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.
Steve
return _context ?? (_context = new Acme.Entities.EntitiesObjectContext())
Is equal to:
if(_context == null)
_context = new Acme.Entities.EntitiesObjectContext();
return _context;
Which implies a double checked locking issue?
So it is.... That just looks so wrong. :)
I certainly hope this wasn't an ASP.Net application.
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.
Side note: It's odd that I don't see anything related to the newly-released NHibernate 3.0.0Alpha1.
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.
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?
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.
Potential Problems:
Bad implementation of a singleton, if indeed that was the intention.
Has potential to cause problems (data integrity and possible race condition) if the same session uses multiple threads.
Static implementation will stay in scope and not garbage collect until the thread shuts down.
Readonly property will not allow a graceful recovery if an exception happens.
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,
Comment preview