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

[Semyon Sadetsky]

Hi Alexey,

On 11/28/2016 11:12 AM, Alexey Ivanov wrote:
> Hi Semyon, Sergey,
>
> On 17.11.2016 11:27, Semyon Sadetsky wrote:
>>
>>
>> On 16.11.2016 20:54, Sergey Bylokhov wrote:
>>> 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.
>> 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.
>
> Semyon is right that the updated values will be written to memory upon 
> exit from synchronized block.
>
> And Sergey is right that the above statement does not guarantee the 
> updated value will be read from memory. That is getter called on 
> another thread could still see the previous value. For getter to read 
> the updated value, it also has to be synchronized (on the same object 
> monitor).
Actually, it requires the same object monitor to keep the synchronized 
contract in full. To reload the local cache any read memory barrier is 
enough to have, for example, synchronized block enter or a Lock object 
lock() call.
The dispatchEventImpl(AWTEvent e) calls Lock.lock() in the beginning. It 
is enough to reload the non-volatile newEventsOnly field.

>
> To avoid synchronized in getters, volatile could be used as it 
> guarantees the reader will always see the latest written value.
>
>> 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.
>
> Unfortunately, you're right other AWT classes are not well-suited for 
> multi-threaded environment…
And they will after the fix.
Anyway, it is poor practices to convert all exiting fields to volatile 
mechanically without any analysis of the issue. Such fix just adds 
unnecessary overhead.
By the way. I removed all added volatile modifiers and run the test 
attached to the fix. The test always passes. It looks like the issue is 
not connected with the fields visibility between threads.

--Semyon
>
>
> Regards,
> Alexey
>
>>>>>> 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.
>> 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.
>>
>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>