On 3/07/2010, at 3:54 PM, Adam Heath wrote: > Scott Gray wrote: >> On 3/07/2010, at 3:03 PM, Adam Heath wrote: >> >>> Scott Gray wrote: >>>> On 3/07/2010, at 2:13 PM, Adrian Crum wrote: >>>> >>>>> --- On Fri, 7/2/10, David E Jones <[email protected]> wrote: >>>>>> On Jul 2, 2010, at 7:11 PM, Adrian Crum wrote: >>>>>> >>>>>>> Because I've done it myself. ;-) >>>>>> Wait, do you mean that in this case you did the actual >>>>>> copying and pasting? Well... I guess that would be one way >>>>>> to know for sure! >>>>> Btw, I'm not implying the code Scott is working on was copy-and-paste >>>>> code - I was just making a generalized statement about where my head is >>>>> at lately. >>>>> >>>>> When I first got involved with Java, I would copy and paste >>>>> double-checked-locking code blocks because I assumed they were necessary. >>>>> I didn't bother to research why they were there. If I had, I would have >>>>> learned that they don't work and there are better ways to do >>>>> synchronization. That is only one example of my uninformed copy-and-paste >>>>> code development - there are more. >>>> This has always bothered me, as far as I can tell double-checked locking >>>> DOES work just fine, the only time it doesn't is when you do the following: >>>> private static Object object; >>>> >>>> public static Object getObject() { >>>> if (object == null) { >>>> synchronized (Some.class) { >>>> if (object == null) { >>>> object = new Object(); >>>> } >>>> } >>>> } >>>> // potentially not fully initialized!! >>>> return object; >>>> } >>>> >>>> But we very rarely use the pattern in that manner. What we do usually is >>>> initialize the object and then in an additional statement we assign it to >>>> the variable being checked. The potential for returning partially >>>> initialized objects is the concurrency book's biggest argument against the >>>> pattern I don't often see that potential in the ways that OFBiz uses it. >>>> >>>> Regards >>>> Scott >>> == >>> if (object = null) { >>> synchronized(lock) { >>> if (object == null) { >>> Object foo = getFoo(); >>> Object bar = getBar(); >>> object = new Result(foo, bar); >>> } >>> } >>> } >>> return object; >>> >>> class Result { >>> Object foo, bar; >>> Result(Object foo, Object bar) { >>> this.foo = foo; >>> this.bar = bar; >>> } >>> } >>> == >> >> Your example is no different than the one I posted. >> >> Are you trying to say that this won't work: >> private static Map cache; >> >> public static Object getObject(String key) { >> if (cache.containsKey(key)) { >> synchronized (cache) { >> if (cache.containsKey(key) { >> Object object = new Object(); >> cache.put(key, object); >> } >> } >> } >> return object; >> } > > Object result = cache.get(key); > if (result == null) { > > If you do a separate containsKey/get, then the cache might have the > object removed between the two calls. But that bug has nothing to do > with DCL. > >> I'm not saying you're wrong, I just want to get an example up of what I >> assumed would work. Btw I never said I had read the book, just pieces. > > Yes, that won't work. > > When the new item is being put into the cache, the map may need to get > resized. This will cause an internal iteration to occur, while the > entries are copied into a new set of buckets. The map will allocate a > new list of buckets, start walking the old version, putting new > objects into the right location. Then, it'll eventually assign the > new bucket list to the active bucket list. > > The vm is free to reorder the memory assignments, so that the > allocated bucket array is assigned to the internal list, before the > items of the array themselves are set. Then another thread will try > to access the map, see the newly allocated bucket list, but not find > any items, because the first thread hasn't copied any yet. > > The only way to protect against this bug is to protect the outer get > call with a synchronized. > > I suggest you go actually read the book. It's very deep stuff.
That's fine, I'll take your word and stand corrected. I actually don't need a nightstand because I'm able to build one out of the stack of great books that are still in queue to be read. The book would have done well to use a more complicated example of why DCL shouldn't be used though. This article seems to do a better better job of it: http://www.ibm.com/developerworks/java/library/j-dcl.html > And yes, I have had to debug problems like this. I've had to rewrite > almost all of commons-vfs, because it didn't have proper locking on > map access, and I had threads enter into a never ending loop. One > thread was doing a get while another did a put, and the looping thread > was stuck inside an interation(kill -QUIT, and a line number, was > enough to verify this bug; the fix was more complex of course).
smime.p7s
Description: S/MIME cryptographic signature
