http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660What am I missing? The docs at msdn.microsoft.com/en-us/library/xfhwa508.aspx say "...where an enumeration contends with write accesses, the collection must be locked during the entire enumeration. To allow the collection to be accessed by multiple threads for reading and writing, you must implement your own synchronization." Are the docs wrong? If not, this code seems to fulfil their requirements. (It can throw exceptions, but "threadsafe" doesn't mean "exception free". I'd assume that fail-fast is what we want.) The ContainsKey method will either get a true hit, a false miss, or a true miss. A true hit is no problem. Either kind of miss will enter the double-checked lock. Assuming we don't have a debugger attached (which can make "lock" theoretically non-thread-safe), the second ContainsKey evaluation will have its collection locked for the entire enumeration, per the docs. There could be a problem if you pass in an object that overrides Equals() in a sketchy way, such that it's not safe for Dictionary use. If two different objects can appear equal, then they'll get the same synchronization object. But that sort of kludge is not "safe" for most uses anyhow. http://ayende.com/4812/code-review-who-can-help-me#comment18http://ayende.com/4812/code-review-who-can-help-me#comment18Thu, 28 Apr 2011 20:58:09 GMTYou are right... it's still breaking at the return... I was sloppy :) http://ayende.com/4812/code-review-who-can-help-me#comment17http://ayende.com/4812/code-review-who-can-help-me#comment17Sat, 16 Apr 2011 04:51:30 GMTJames, My problem with the mapping is that we can't really see what is going on. It is broken all over the place. I would rather see it all in one place so that I can follow it. http://ayende.com/4812/code-review-who-can-help-me#comment16http://ayende.com/4812/code-review-who-can-help-me#comment16Sat, 16 Apr 2011 04:43:26 GMTfirefly, No, it cannot be made thread safe as written http://ayende.com/4812/code-review-who-can-help-me#comment15http://ayende.com/4812/code-review-who-can-help-me#comment15Sat, 16 Apr 2011 04:41:41 GMTIt will be thread safe if the dictionary is declared as volatile... still I have no idea what this class suppose to do. http://ayende.com/4812/code-review-who-can-help-me#comment14http://ayende.com/4812/code-review-who-can-help-me#comment14Sat, 16 Apr 2011 04:27:46 GMT@RJ They are storing objects as keys in a static dictionary. Even if every other reference to the object goes out of scope, they dictionary will still hold a reference as a key, and the dictionary is never cleared out. So anything added to the dictionary, which is any object you call LockableObjectProvider.GetLockableObject() on, will never be garbage collected. http://ayende.com/4812/code-review-who-can-help-me#comment13http://ayende.com/4812/code-review-who-can-help-me#comment13Fri, 15 Apr 2011 19:30:11 GMTThe one question I've got is how does one avoid the ever growing service with an implementation like this? It seems like the NewsService could end up with quite a lot of code being so very procedural. I'm not arguing for abstraction.. but a little OO would be nice no? http://ayende.com/4812/code-review-who-can-help-me#comment12http://ayende.com/4812/code-review-who-can-help-me#comment12Fri, 15 Apr 2011 17:07:13 GMT"One thing that did bother me is the amount of mapping that is going on there, and how much of that I was simply unable to follow." How would you prefer to handle the mapping? Is your concern that the mapping for the SearchResultsPageViewModel doesn't happen in one place? http://ayende.com/4812/code-review-who-can-help-me#comment11http://ayende.com/4812/code-review-who-can-help-me#comment11Fri, 15 Apr 2011 16:39:58 GMTWhere is the memory leak? http://ayende.com/4812/code-review-who-can-help-me#comment10http://ayende.com/4812/code-review-who-can-help-me#comment10Fri, 15 Apr 2011 15:04:54 GMTDamien :: In general. no - you shouldn't be locking the actual object under usage, because it's not fine grained enough and if other consumers are also doing similar you can end up with deadlocks occurring over unrelated functionality that shouldn't involve deadlocks. That said, the above implementation ALSO has that problem - you should *really* perform locks on specific data for specific functionality, and not perform locks on exposed functional objects. http://ayende.com/4812/code-review-who-can-help-me#comment9http://ayende.com/4812/code-review-who-can-help-me#comment9Fri, 15 Apr 2011 11:50:07 GMTpublic static object GetLockableObject(object o) { return o; } surely? http://ayende.com/4812/code-review-who-can-help-me#comment8http://ayende.com/4812/code-review-who-can-help-me#comment8Fri, 15 Apr 2011 11:03:52 GMTI'm probably taking this off topic (into implementation details of dictionary and its internal data structures) .. probably sufficient to say "it's calling methods from multiple threads that aren't guaranteed to be thread safe" and leave it at that :-) http://ayende.com/4812/code-review-who-can-help-me#comment7http://ayende.com/4812/code-review-who-can-help-me#comment7Fri, 15 Apr 2011 09:58:11 GMTYes, on the face of it, this is safe code, in the sense that we will never get DuplicateKeyException. But the implementation of ContainsKey() isn’t safe to run when Add() is also executing. The actual behavior depends on the implementation, but it is easy enough to imagine a scenario where invariants that ContainsKey() relies on are broken for the duration of the Add() call. http://ayende.com/4812/code-review-who-can-help-me#comment6http://ayende.com/4812/code-review-who-can-help-me#comment6Fri, 15 Apr 2011 09:52:59 GMT@Patrick ah - it's a dictionary problem, fair enough, I was looking at the general structure rather than what it was actually doing. Thanks. Out of interest, how does it "break" ? It should never (theoretically) be calling add and get concurrently if ContainsKey doesn't return true until the item is actually added, and not when it's halfway though adding it. http://ayende.com/4812/code-review-who-can-help-me#comment5http://ayende.com/4812/code-review-who-can-help-me#comment5Fri, 15 Apr 2011 09:51:08 GMTSo, a ConcurrentDictionary ( [msdn.microsoft.com/en-us/library/dd287191.aspx](http://msdn.microsoft.com/en-us/library/dd287191.aspx)) could be used instead if the double-locking ... http://ayende.com/4812/code-review-who-can-help-me#comment4http://ayende.com/4812/code-review-who-can-help-me#comment4Fri, 15 Apr 2011 09:44:27 GMTOh, and the same goes for concurrently using Get and Add. Although thinking about it, maybe ContainsKey won't break, but Get definitely can. http://ayende.com/4812/code-review-who-can-help-me#comment3http://ayende.com/4812/code-review-who-can-help-me#comment3Fri, 15 Apr 2011 09:39:03 GMT@Steven Robbins You can't use ContainsKey concurrently with Add. When Add changes the underlying structure, ContainsKey will break. http://ayende.com/4812/code-review-who-can-help-me#comment2http://ayende.com/4812/code-review-who-can-help-me#comment2Fri, 15 Apr 2011 09:36:58 GMTAs silly as LockableObjectProvider is (hard to believe the developer didn't spot the "memory leak" problem), I don't see why it's not thread safe? Looks like a standard double check lock - so won't work in Java land, but should work ok on the MS CLR. Not that I'm advocating it, just curious :-) http://ayende.com/4812/code-review-who-can-help-me#comment1http://ayende.com/4812/code-review-who-can-help-me#comment1Fri, 15 Apr 2011 09:22:08 GMT