--- On Fri, 7/2/10, Scott Gray <[email protected]> wrote: From: Scott Gray <[email protected]> Subject: Re: svn commit: r959875 - /ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ModelWidgetAction.java To: [email protected] Date: Friday, July 2, 2010, 8:36 PM
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; } 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. ----------------------------------------------- In Adam's example, it is entirely possible that object = new Result(foo, bar); will be executed before Object foo = getFoo(); Object bar = getBar(); ----------------------------------------------- > > This is buggy too, and is a much more common pattern. In fact, this > is the only argument needed to remove all DCL patterns. > > When a new object is created, a memory block is allocated. The > address of that block is then stored in object. The memory block is > then initialized. The block is not fully initialized until the > constructor is returned from. > > After the address of the block is stored in the object, then the outer > conditional thinks the object is no longer null. This is true. But > != null does not mean the object has been initialized yet. > > If you try to assign the new object to a tmp first, then assign to the > final output, it won't help, because the memory assignment could still > be reordered. All memory read/writes that occur inside a synchronized > block can be reordered in any fashion. One of those memory accesses > is the storage of the allocated objects memory address into the > unprotected object. If the memory access can be reordered, then it's > possible that the instances variable assignments could happen after > the object assignment, which then means the returning of a non-null, > uninitialized object. > > Yes, it's complex. If you don't understand my ramblings(the larger > paragraph), then you need to go back and re-read the concurrency book. >
