On 17.11.16 11:27, Semyon Sadetsky wrote:
The statement above is incorrect, there is no "cache". I do not know
where you get "changed fields is guaranteed to be flushed upon exit
from the synchronized block". Also there is no guarantee that the
reader will see the latest version of the field if the reader will use
another mutex or will not use synchronization at all. In the fix
"volatile" will guarantee that the readers will see the latest version
which was set.
Flushing the cache is reality. By adding volatile to all Menu* fields
you didn't make the methods effects predictable in case of they are
called concurrently because they are not synchronized between each
other. After this change it still may not be recommended to use menus
from different threads arbitrarily.
all above is wrong.
Or if I'm wrong and this change is a real improvement why you don't add
volatile to all AWT classes' fields?
The rest AWT classes are not better synchronized than the menu onces.
There is a bug for all other components.
https://bugs.openjdk.java.net/browse/JDK-6765536
We have a setter and we call it in applets, trayicon and in X11.
But TryIcon is not a MenuComponent. It seems the comment is correct.
But we still have a setter which is called by other code. Also it
cannot be made final because it is updated during de-serialization.
And you still did not answer where it is really called. Probably the
setter may be removed? At least please update the comment statement
since in the change you assumes that the opposite is true.
Even if the setter will be removed the field cannot be final because it
is updated during de-serialization. I will remove the comment about
constructor before the push.
Why you left without synchronization the analogous field in the
Component class? This field is really modified I can give you examples.
Also it seems Menu#isHelpMenu field is never used except for
toString()
and may be removed.
Why it can be removed since it is used in the toString()?
Because in this case it looks like cache anti-pattern and it should be
replaced with the real value. For which purpose the toString() may be
used? for debugging? But it seems one cannot guarantee that the cache is
updated in each moment of time in case of multi-threading.
It is used to provide an information in the "string" that this menu is
a helpmenu. It is used in some tests as well. We also should take care
since this object is Serializable.
--Semyon
- When the submenu is removed from Menu/MenuBar we do not reset
its
parent field if the Menu/MenuBar have peer==null. So if later we
tried
to call MenuBar.setHelpMenu(submenu) we skip this submenu
because we
think it was added already.
Bug: https://bugs.openjdk.java.net/browse/JDK-8165769
Webrev can be found at:
http://cr.openjdk.java.net/~serb/8165769/webrev.00
--
Best regards, Sergey.
