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
