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

Tue Sep 8 10:31:24 UTC 2015

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.

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

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

-- 
Best regards, Sergey.