<AWT Dev> <Swing Dev> Review request #4: 6852592 (invalidate() must be smarter) - approved

[Alexander Potochkin]

Hello Anthony

>> There are enough serious reasons why not to use it,
>> I have read them in a "Java Concurrency" book
>> which is alway on my table
> 
> That's really surprising to hear from a developer of a single-threaded 
> GUI-toolkit! We, AWT people, should have this book instead! ;)

:-)

> 
> Now back to serious matters, at first I thought you were talking about 
> the presence of the descendUnconditionallyWhenValidating variable 
> itself, that's why I asked for elaboration. Now I see that basically 
> you're speaking about the double-checked locking issue only. Thanks for 
> clarifying that.
> 
> Indeed, Artem noticed this anti-pattern long-long time ago. Even before 
> the new variable is introduced, the idiom involving the bare isValid() 
> check has existed in our code for ages. And I agree that we need to get 
> rid of that idiom.

I'd recommend to get rid of the bare isValid() outside the synchronized 
block as well

> 
> However, the current fix for 6852592 does not seem to make things worse: 
> it just adds one more check that is basically equal (in possible 
> side-effects) to the existing check for isValid(). On the other hand, 
> making that change may theoretically bring some regressions if some code 
> relies on that behavior. That's why I decided to separate the issues. 
> Now we have the following CR:
> 
> http://bugs.sun.com/view_bug.do?bug_id=6887249

Yep, I see that you propose exactly the same in this CR

> 
> The fix is actually in progress, and apparently going to hit the 
> repository prior to the fix for 6852592 (due to a bit lagging 
> specification changes approval process). So I don't have any concern 
> regarding the issue in terms of the fix that we're currently reviewing: 
> at least the regression testing indicates nothing extremely terrible. I 
> hope this explanation calms down your worries. If not, please speak up.

I am just not comfortable with having a kind of a half-done fix in JDK,
however I see that there are several more instance of that antipattern 
in the Container class, see preferredSize(), minimumSize() getMaximumSize()

are you going to fix them as well in #6887249?

If so I'll approve this one
;-)

Thanks
alexp

> 
> -- 
> best regards,
> Anthony
> 
>>
>> Moreover your variant is even more risky:
>> any field that is going to be accessed via different threads
>> must always be used under the lock,
>> otherwise it is possible that one thread
>> will see the field in an invalid state
>>
>> However in your fix the descendUnconditionallyWhenValidating  field
>> is mutated under the treeLock and then accessed in the validate() method
>> outside the synchronization block which is unsafe
>>
>> The following comment: /* Avoid grabbing lock unless really necessary.*/
>>
>> is obsolete, the modern JVM do a great job to optimize synchronization
>> so there is no need to invent tricky patterns to avoid synchronization
>>
>> I see two options here:
>>
>> 1) Refactor Container.validate to alway get the treeLock and
>> do everything inside it. This is the most preferable choice.
>>
>> 2) Define descendUnconditionallyWhenValidating as volatile,
>> this is better than nothing but less safe than #1
>>
>> Thanks
>> alexp
>>
>>>
>>> -- 
>>> best regards,
>>> Anthony
>>>
>>>>> On 10/01/2009 12:24 PM, Artem Ananiev wrote:
>>>>>> I've quickly looked through the changes, the fix looks fine with 
>>>>>> the exception of using double-check idiom in Container.validate() 
>>>>>> - I told you about that the other day.
>>>>>
>>>>> To make sure we don't get messed up when discovering potential 
>>>>> regressions, I've just filed a separate CR 6887249 (Get rid of 
>>>>> double-check for isValid() idiom in validate() methods) to fix the 
>>>>> issue later. Thanks for the suggestion.
>>>>>
>>>>> -- 
>>>>> best regards,
>>>>> Anthony
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Artem
>>>>>>
>>>>>> Anthony Petrov wrote:
>>>>>>> It's been a long time since we discussed the issue. Now is the 
>>>>>>> time for revival.
>>>>>>>
>>>>>>> Last time we came across a failing test [1] that had a JApplet 
>>>>>>> embedded in a JFrame. The frame was expected to be validated upon 
>>>>>>> showing. However, the components in the JApplet were not 
>>>>>>> validated, since the JApplet itself was marked valid, but the 
>>>>>>> invalidate() requests from the children of the applet stopped on 
>>>>>>> the RootPane of the JApplet because it was a validate root.
>>>>>>>
>>>>>>> Later we found out a possible solution for that problem [2]: the 
>>>>>>> show() (as well as the pack()) should validate the whole 
>>>>>>> component hierarchy unconditionally.
>>>>>>>
>>>>>>> So, here's the fix with this solution implemented. Please review:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~anthony/7-23-invalidate-6852592.3/
>>>>>>>
>>>>>>> The fix has been tested quite thoroughly: all sort of related 
>>>>>>> automatic tests for both Swing and AWT areas have been run 
>>>>>>> (including layout-related tests, bare (J)Component and 
>>>>>>> Container-related tests, and some other.) All manual 
>>>>>>> layout-related tests from AWT and Swing have also been run and 
>>>>>>> passed. Mixing-related regression tests pass as well.
>>>>>>>
>>>>>>> Please note that I've also changed the synopsis of the change 
>>>>>>> request by replacing revalidate() with invalidate() because the 
>>>>>>> fix actually affects the invalidate() method only.
>>>>>>>
>>>>>>> [1] 
>>>>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2009-August/000831.html 
>>>>>>>
>>>>>>>
>>>>>>> [2] 
>>>>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2009-August/000835.html 
>>>>>>>
>>>>>>>
>>>>>>> -- 
>>>>>>> best regards,
>>>>>>> Anthony
>>>>
>>