http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660I 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 http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment29http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment29Wed, 28 Jul 2010 05:55:22 GMTPotential 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. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment28http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment28Tue, 27 Jul 2010 12:55:25 GMTIt 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. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment27http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment27Mon, 26 Jul 2010 16:00:50 GMTOk, 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? http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment26http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment26Mon, 26 Jul 2010 13:21:01 GMTWith 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. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment25http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment25Mon, 26 Jul 2010 06:48:30 GMTSide note: It's odd that I don't see anything related to the newly-released NHibernate 3.0.0Alpha1. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment24http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment24Mon, 26 Jul 2010 05:32:12 GMTWith 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. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment23http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment23Mon, 26 Jul 2010 03:17:21 GMTI certainly hope this wasn't an ASP.Net application. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment22http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment22Mon, 26 Jul 2010 03:12:54 GMTSo it is.... That just looks so wrong. :) http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment21http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment21Mon, 26 Jul 2010 02:53:33 GMTWhich implies a double checked locking issue? http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment20http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment20Mon, 26 Jul 2010 02:08:24 GMTSteve return _context ?? (_context = new Acme.Entities.EntitiesObjectContext()) Is equal to: if(_context == null) _context = new Acme.Entities.EntitiesObjectContext(); return _context; http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment19http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment19Sun, 25 Jul 2010 23:38:34 GMTAs 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. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment18http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment18Sun, 25 Jul 2010 23:36:44 GMTThilak & gunteman, Precisely! http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment17http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment17Sun, 25 Jul 2010 23:35:37 GMTShlomo, Fixed, thanks http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment16http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment16Sun, 25 Jul 2010 23:33:05 GMTGreg, Yes, it does. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment15http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment15Sun, 25 Jul 2010 23:32:31 GMTAlberto, 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. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment14http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment14Sun, 25 Jul 2010 23:32:14 GMTConfigurator, Thread Safety is a problem, sure. But let us assume that we are talking about single threaded code, it is still wrong, why? http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment13http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment13Sun, 25 Jul 2010 23:31:03 GMTThe 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 http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment12http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment12Sun, 25 Jul 2010 21:57:36 GMT> 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. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment11http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment11Sun, 25 Jul 2010 21:07:22 GMTYeah, 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. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment10http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment10Sun, 25 Jul 2010 20:08:00 GMTThread unsafe work with shared variable . In such situation it's possible that concurrent clients get diffrent instances of the context class. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment9http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment9Sun, 25 Jul 2010 19:46:03 GMTNice :) 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 :) http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment8http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment8Sun, 25 Jul 2010 19:37:40 GMTIt’s not even a singleton. This code shows that EntitiesObjectContext may have internal/ public constructor. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment7http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment7Sun, 25 Jul 2010 19:24:59 GMTthis is why I never catch that @#$@ bird http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment6http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment6Sun, 25 Jul 2010 18:26:07 GMTObject contexts are reasonably volatile resources... like NH sessions. Not a good idea to singleton them. They are meant for short lived usages. http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment5http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment5Sun, 25 Jul 2010 17:24:53 GMTThere's a missing closing curly brace, though I assume that's a typo http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment4http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment4Sun, 25 Jul 2010 16:31:53 GMTDoes the 2nd half of the ?? Operator even return the correct type? http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment3http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment3Sun, 25 Jul 2010 16:13:11 GMTCould it be a race condition where more than one context gets created? http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment2http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment2Sun, 25 Jul 2010 15:56:43 GMTThread 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(); } ? http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment1http://ayende.com/4561/find-the-bug-accidental-code-reviews#comment1Sun, 25 Jul 2010 15:49:42 GMT