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.
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).
