<AWT Dev> [9] Review Request: 8165769 Hang in the help menu item

Sergey Bylokhov Sergey.Bylokhov at oracle.com
Wed Nov 16 17:54:43 UTC 2016


On 16.11.16 20:25, Semyon Sadetsky wrote:
>> The example above produce the same result as if the thread B will call
>> dispatchEventImpl() early than addItemListener() was called by thread
>> A. And this is correct behavior(the new events will be proceeded only
>> when we set newEventsOnly to true).
>> addItemListener is synchronized, because we need to synchronize access
>> to the list of listeners "itemListener" when we add/remove listener.
> Then explain to me why to make all those fields volatile? Because the
> cache for the changed fields is guaranteed to be flushed upon exit from
> the synchronized block, so the changes will be visible to other threads
> when the method returns.

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.


>>> Same for enableEvents(long eventsToEnable) method.
> Also, the state field setter is synchronized by this object monitor
> while the peer object which it should notify is protected by another
> monitor and may be reset concurrently.
>>
>>>> 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.
>>
>> 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.

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


More information about the awt-dev mailing list