On 16.11.2016 17:52, Sergey Bylokhov wrote:
On 16.11.16 17:32, Semyon Sadetsky wrote:
There are a few issues which can cause a hang in some of our menu
stress tests:
- Lack in synchronization in the Menu, we tried to synchronized the
write of fields, but skip synchronization when we read the fields(in
getters for example). In the fix most of the fields were marked as
final(if possible) or volatile.
Since the synchronized blocks cannot be simply omitted the adding
volatile keyword to the fields doesn't improve multithreading capability
of menu components and the most of the proposed changes practically
useless.
Please clarify the statement above.
I can give you an example:
CheckboxMenuItem.java
243 public synchronized void addItemListener(ItemListener l) {
244 if (l == null) {
245 return;
246 }
247 itemListener = AWTEventMulticaster.add(itemListener, l);
248 newEventsOnly = true;
249 }
we are enabling event by calling the method above on thread A. The
method is synchronized.
at the same time on thread B dispatchEventImpl(AWTEvent e) is called
which is not synchronized at all.
If thread A is paused by the scheduler at line 248 then thread B may be
executed and the event will be skipped in dispatchEventImpl(AWTEvent e)
because of newEventsOnly=false still.
By making newEventsOnly and itemListener fields volatile you only
guarantee their visibility for thread B but this doesn't make
newEventsOnly change atomic along with adding an item listener.
Same for enableEvents(long eventsToEnable) method.
Changing package private accesses to private in general improves
encapsulation and may be approved. But multithreading issues should be
fixed differently.
Actually the MenuComponent appContext field is never changed according
to its spec:
/**
* The {@code AppContext} of the {@code MenuComponent}.
* This is set in the constructor and never changes.
*/
transient AppContext appContext;
I suggest to declare it final and also this will be enough to make it
thread safe.
But in some corner cases we change this value, so it cannot be final.
What is that corner case? The comment clearly states that it is never
changed.
Note that Component appContext field can be changed and this
multithreading issue should be resolved. Since the filed is accessed
only using the component accessor it should be enough to synchronize the
corresponding getter and setter.
Also it seems Menu#isHelpMenu field is never used except for toString()
and may be removed.
--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
