RFR 8160892: VM warning: WaitForMultipleObjects timed out

[Daniel D. Daugherty]

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
>