<AWT Dev> <Awt Dev> [9] Review Request for 8132664: closed/javax/swing/DataTransfer/DefaultNoDrop/DefaultNoDrop.java locks on Windows

Tue Sep 8 10:42:16 UTC 2015


On 9/8/2015 1:31 PM, Sergey Bylokhov wrote:
> On 08.09.15 13:15, Semyon Sadetsky wrote:
>>
>>
>> On 9/8/2015 12:33 PM, Sergey Bylokhov wrote:
>>> On 08.09.15 12:04, Semyon Sadetsky wrote:
>>>>
>>>>
>>>> On 9/7/2015 6:56 PM, Sergey Bylokhov wrote:
>>>>> On 07.09.15 16:41, Semyon Sadetsky wrote:
>>>>>>
>>>>>>
>>>>>> On 9/7/2015 3:28 PM, Sergey Bylokhov wrote:
>>>>>>> On 03.09.15 17:43, Semyon Sadetsky wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/5/2015 2:33 PM, Sergey Bylokhov wrote:
>>>>>>>>> On 05.08.15 14:20, Semyon Sadetsky wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 8/5/2015 1:39 PM, Sergey Bylokhov wrote:
>>>>>>>>>>> On 05.08.15 13:18, Semyon Sadetsky wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 8/5/2015 12:27 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>> On 04.08.15 14:54, Semyon Sadetsky wrote:
>>>>>>>>>>>>>> On 8/3/2015 6:05 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>>>> Hi, Semyon
>>>>>>>>>>>>>>> Did you try to change dwMilliseconds from INFINITE to the
>>>>>>>>>>>>>>> timeout(10 seconds by default?) which is passed to the
>>>>>>>>>>>>>>> method?
>>>>>>>>>>>>>>> It does not help? Because even when dnd is not used we 
>>>>>>>>>>>>>>> should
>>>>>>>>>>>>>>> not wait event for infinite time.
>>>>>>>>>>>>>> It would not help to fix the issue because 10 seconds is
>>>>>>>>>>>>>> too big
>>>>>>>>>>>>>> interval. But for consistency it is not bad to have a time
>>>>>>>>>>>>>> limit.
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8132664/webrev.01/
>>>>>>>>>>>>> Note that syncNativeQueue is intended to wait until the 
>>>>>>>>>>>>> native
>>>>>>>>>>>>> event queue is flushed or until timeout will expire. So 
>>>>>>>>>>>>> even if
>>>>>>>>>>>>> timeout is expired we collect the native events during this
>>>>>>>>>>>>> period
>>>>>>>>>>>>> of time.Can you double check that the event counter is
>>>>>>>>>>>>> incremented
>>>>>>>>>>>>> during dnd? I do not know how we block the toolkit thread,
>>>>>>>>>>>>> probably we create some nested loops which ignore our event
>>>>>>>>>>>>> posted
>>>>>>>>>>>>> from syncNativeQueue, can we change that?
>>>>>>>>>>>> Yes, this is an internal secondary loop which waits for mouse
>>>>>>>>>>>> release event.
>>>>>>>>>>> Can we change the condition and process the sync event in this
>>>>>>>>>>> loop?
>>>>>>>>>> Why? Will receive all events on the toolkit thread when 
>>>>>>>>>> doDragDrop
>>>>>>>>>> returns.
>>>>>>>>>
>>>>>>>>> When how we get dragenter/exit events?
>>>>>>>>
>>>>>>>> On the platform side they are not events but callbacks which are
>>>>>>>> converted into events on the java side and added to the queue. So
>>>>>>>> they
>>>>>>>> will be detected by isEQEmpty() and waitForIdle() will be 
>>>>>>>> repeated.
>>>>>>>
>>>>>>> This is not directly related to this fix. Most of our
>>>>>>> callbacks/events
>>>>>>> posts events to the EQ, but there is a possibility for a lag 
>>>>>>> between
>>>>>>> callback and a post events, and this is why the syncNativeQueue was
>>>>>>> added.
>>>>>>> Let's return to the beginning: the syncNativeQueue method
>>>>>>> according to
>>>>>>> its specifications should try to flush the native system, track the
>>>>>>> native activity and should not wait more than timeout parameter.
>>>>>>
>>>>>> It is not possible to send sync events to the DD loop. Only
>>>>>> mouse/keyboard event can return control to the app. I would not
>>>>>> emulate
>>>>>> them in the syncnative.
>>>>>> Always waiting the max time is not good. Maybe wait a bit and 
>>>>>> check if
>>>>>> no callbacks happened?
>>>>>
>>>>> It will be good to check that callbacks were occurred in the same way
>>>>> as native events. This can be done in this fix. As for question about
>>>>> wait or not when timeout is over, is a shared problem, which affect
>>>>> the osx also. It can be fixed separately. I guess the logic in
>>>>> realSync can be tweaked and the default timeout(10*4 sec) is not a
>>>>> optimal as a default value, 2 or 3 seconds without any events should
>>>>> be enough.
>>>> Could you explain your logic?
>>>
>>> My logic is to follow the specification.
>> I cannot accept this as an answer because it is an abstract declaration.
>> Please, try to be more specific.
>
> Just implement as I described it above.
>
Sorry?
>
>> I can only make guesses what specification section you mean. For
>> example, the next is waitForIdle() method's spec:
>>      /**
>>       * Waits until all events currently on the event queue have been
>> processed.
>>       * @throws  IllegalThreadStateException if called on the AWT event
>> dispatching thread
>>       */
>> What does specifically contradicts to this specification?
>
> Check the documentation of the realSync method.
>
realSync is an internal method. It is not a part of the specification.
It has this description of the timeout parameter:
      * @param timeout the maximum time to wait in milliseconds, 
negative means "forever".

That is as it used in the fix. No?
>
>>>
>>>> Why to wait short wait period doesn't work
>>>> if we are in DD and there are no callbacks?
>>>
>>> "No callbacks" during what period? My point: this period should be
>>> passed to this method.
>> Period is passed to the method.
>> Java_sun_awt_windows_WToolkit_syncNativeQueue(JNIEnv *env, jobject self,
>> jlong timeout)
>> It's meaning is a _maximum_ time the method should wait. Before the fix
>> it was ignored and the method never returned.
>> With the fix the timeout is taken into account but this aspect of the
>> fix is not related to the original issue and it cannot fix the issue 
>> alone.
>> Or you meant another method? Again, please be more specific otherwise
>> this discussion can be infinite.
>>>
>>>> No callbacks means no
>>>> activity and in analogous situations the method returns true.
>>>
>>>
>>>
>>>> I see a clear disadvantage: even with 2-3 second wait period the test
>>>> will executes much longer than on Linux platform because D&D is
>>>> triggered every 3 mouse drags . Also other D&D tests suites will 
>>>> demand
>>>> much more time to pass the windows platform. Users can report bugs if
>>>> AWT Robot starts to produce multi-seconds delay after drag events.
>>>> Maybe you can provide an example scenario which demonstrates that 
>>>> it is
>>>> really necessary to pay this price?
>>>>
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>> Event counter is not changed during toolkit thread blocking of
>>>>>>>>>>>> cause. Not sure that we can change that. But since toolkit
>>>>>>>>>>>> queue is
>>>>>>>>>>>> blocked we can assume that we are synced.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The timeout value is maintained on the shared level and
>>>>>>>>>>>>> actually
>>>>>>>>>>>>> this test will fail with timeout on osx as well JDK-7185258.
>>>>>>>>>>>>> The
>>>>>>>>>>>>> test will fail even if the time out will be changed to 
>>>>>>>>>>>>> ±100ms,
>>>>>>>>>>>>> because it call realsync on each pixel move, ±200 times. This
>>>>>>>>>>>>> problem can be fixed in different ways like tweak of
>>>>>>>>>>>>> timeouts and
>>>>>>>>>>>>> numbers of iterations, or changing the test to call w4idle 
>>>>>>>>>>>>> only
>>>>>>>>>>>>> after the latest move(actually I think this method can be
>>>>>>>>>>>>> moved to
>>>>>>>>>>>>> the robot class).
>>>>>>>>>>>>>
>>>>>>>>>>>>> So I still think that the right fix for a deadlock, which is
>>>>>>>>>>>>> subject of this bug, is simply change the syncNativeQueue and
>>>>>>>>>>>>> waits using a timeout if it is positive, and waits forever if
>>>>>>>>>>>>> timeout is negative (the same bug on osx JDK-8080504).
>>>>>>>>>>>> I'm not sure that waiting brings any value. What do you
>>>>>>>>>>>> propose to
>>>>>>>>>>>> return if it timed out? The event counter will not be changed
>>>>>>>>>>>> regardless of waiting.
>>>>>>>>>>> But it should be changed, because we get native events from the
>>>>>>>>>>> system during dnd and in each such callback we should update 
>>>>>>>>>>> this
>>>>>>>>>>> counter. If callbacks were not called=>counter was not updated
>>>>>>>>>>> then
>>>>>>>>>>> sync assume that currently we do not process events. If 
>>>>>>>>>>> callbacks
>>>>>>>>>>> were called then sync assume that we have events in the native
>>>>>>>>>>> queue
>>>>>>>>>>> and should try to sync again on the next iteration.
>>>>>>>>>> No. Events are not processed while toolkit is blocked in
>>>>>>>>>> doDragDrop(). The application state is frozen on that period.
>>>>>>>>>> That is
>>>>>>>>>> Windows approach.
>>>>>>>>>>>
>>>>>>>>>>>> With such waiting the test will fail because of either jtreg
>>>>>>>>>>>> timeout
>>>>>>>>>>> Default timeout is 120 seconds for everything, the test try to
>>>>>>>>>>> sync
>>>>>>>>>>> the queue 200 of times after each move, so yes it can fail with
>>>>>>>>>>> timeout even if spend in nativesync 200 ms, the possible
>>>>>>>>>>> solutions
>>>>>>>>>>> were in my previous email.
>>>>>>>>>>>> either InfiniteLoop exception.
>>>>>>>>>>> This exception will be disabled by default lately in jdk9
>>>>>>>>>>> timeframe,
>>>>>>>>>>> right now it helps to find some related issues.
>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 03.08.15 17:26, Semyon Sadetsky wrote:
>>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Please review fix for JDK9:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8132664
>>>>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8132664/webrev.00/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> DoDragDrop() is blocking, so upon drag operation is
>>>>>>>>>>>>>>>> triggered
>>>>>>>>>>>>>>>> the toolkit thread is blocked and the WM_AWT_WAIT 
>>>>>>>>>>>>>>>> cannot be
>>>>>>>>>>>>>>>> processed which in its turn blocks the AWT robot.
>>>>>>>>>>>>>>>> The solution is to escape AWT robot waiting in
>>>>>>>>>>>>>>>> syncNativeQueue() if drag operation is in progress.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20150908/ffd8ae6a/attachment-0001.html>