From: Caleb Clausen <vikkous@...>
Date: 2010-05-10T02:16:54+09:00
Subject: [ruby-core:30119] Re: [Bug #2488] thread usage can result in bad 	HANDLE

On 5/9/10, wanabe <s.wanabe@gmail.com> wrote:
>> Why is it necessary to re-check that intr is set to what it was set to
>> just two lines before? I assume it might be changed by the action of
>> another thread, but if that's the case, wouldn't it be better to just
>> check that field once while inside the global_vm_lock? (ie move the
>> lock/unlock to be around the whole if statement.)
>
> Yes, you are right.
> I think first that it is good to avoid native_mutex_lock() as much as
> possible.
> Now, I realize the condition (th && !intr) is fulfilled in a only few
> moments.
>
> But if we delete check of outside, new check of mutex's existence will
> be needed.
> I'm afraid that it doesn't make sense from the standpoint of cost down.

The mutex in this case is global_vm_lock, which always should exist...
shouldn't it? I think you perhaps meant 'new check of
interrupt_event's existence'?

>> For that matter, shouldn't ruby be using the api provided by windows
>> (WaitForSingleObject, apparently) to check the state of this 'event
>> object' (really a semaphore)? If you do it that way, then maybe the
>> global_vm_lock doesn't need to be touched.
>
> I guess it is not very good idea.
> Because th->native_thread_data.interrupt_event can be destroyed
> by native_thread_destroy() between check and lock, but GVL can't.
>
> And if ruby use  'event object', we should insert locking to somewhere
> such as thread_cleanup_func(), but lock with GVL is in thread_start_func_2()
> already. There is not a new cost.

My concern was that interrupt_event is already an event object... But
I misunderstood the intent of accessing the interrupt_event field in
w32_wait_events. I thought it was seeing if any event had been
signalled, but now from your comments, it's clear that it's just
seeing if the event object still exists. Thank you for helping me
understand this. My knowledge of this code is partial; please forgive
my ignorance.

> But if you know the use case the lock can be harm, I'm glad to change it.

Your changes to w32_wait_event might conflict with my attempt to make
thread priorities work again. I need to change all uses of
global_vm_lock to something else. Which is why I was trying to think
of an excuse to get rid of the extra global_vm_lock usage you had
created. Your comments have helped me understand what you did better,
now I see I was going in the wrong direction. I still have to figure
out what to do in w32_wait_event, but you've given me some other
options to think about.