RFR 8160892: VM warning: WaitForMultipleObjects timed out

[Ivan Gerasimov]

Thanks Daniel!

I'll rearrange comments as you suggest.

With kind regards,
Ivan


On 07.07.2016 0:21, Daniel D. Daugherty wrote:
> On 7/6/16 10:57 AM, Ivan Gerasimov wrote:
>> Hello!
>>
>> When fixing JDK-8069048 a potential race was overlooked:
>> 1) Thread A wants to call _endthreadex() and registers itself,
>> 2) Thread B wants to call exit(), takes its turn and raises the flag 
>> `process_exiting`,
>> 3) Thread A continues, and gets captured in the loop at 4020, in 
>> SuspendThread(GetCurrentThread()),
>> 4) Now, the thread B will have to wait for all the registered 
>> threads, including the thread A, which will never wake up.
>>
>> Would you please help review the fix?
>>
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8160892
>> WEBREV: http://cr.openjdk.java.net/~igerasim/8160892/00/webrev/
>
> src/os/windows/vm/os_windows.cpp
>     Just to make sure the race is clear (in my mind at least):
>
>     Thread A
>       - executes if-block at L3913-3968; in particular it
>         registers itself on L3957
>       - executes if-block at L4017-4027; in the old code,
>         the thread would block for ever in L4022-4026; in
>         the new code, the thread will only block if it did
>         not register itself.
>
>     Thread B:
>       - executes if-block at L3906-3910 and succeeds in
>         setting the process_exiting flag
>       - executes if-block at L3969-4012; in particular
>         Thread B calls WaitForMultipleObjects() on L3994;
>         in the old code, WaitForMultipleObjects() times
>         out because Thread A is blocked.
>
>     L3963:           registered_itself = true;
>     L3964:         }
>     L3965:
>     L3966:         // The current exiting thread has stored its handle 
> in the array, and now
>     L3967:         // should leave the critical section before calling 
> _endthreadex().
>         The existing comment on L3966-3967 belongs after L3963 and before
>         L3964, i.e., at the end of the else-block.
>
>         If you agree with moving the comment on L3966-3967, then
>         consider this next request:
>
>     L3959:           warning("DuplicateHandle failed (%u) in %s: %d\n",
>     L3960:                   GetLastError(), __FILE__, __LINE__);
>
>         Please consider adding the following comment after the warning:
>
>         // We can't register this thread (no more handles) so this thread
>         // may be racing with a thread that is calling exit(). If the 
> thread
>         // that is calling exit() has managed to set the process_exiting
>         // flag, then this thread will be caught in the SuspendThread()
>         // infinite loop below which closes that race. A small timing
>         // window remains before the process_exiting flag is set, but it
>         // is only exposed when we are out of handles.
>
>     L4021:       // don't let the current thread proceed to exit() or 
> _endthreadex()
>         Clarify comment: 'current thread'
>                       -> 'current unregistered thread'
>
> Very nice and very quick job in finding this race!
>
> Thumbs up on what you have since my comments are only related
> to moving/adding comments.
>
> Dan
>
>
>>
>> With kind regards,
>> Ivan
>>
>
>