[Mono-dev] Re: Please Help : Leak of wapi handles

[Jonathan Gilbert]

At 06:41 PM 06/05/2006 -0400, Miguel de Icaza wrote:
>Hello,
>
>> You probably mean this fix:
>> 
>> http://lists.ximian.com/pipermail/mono-patches/2006-May/074236.html
>> 
>> It doesn't fix the thread handles ref leaks I experienced.
>> 
>> I was able to fix it by changing io-layer/threads.cs:GetCurrentThread
>> to not ref the handle. I'm not sure about the side effects, still
>> testing.
>
>Ah, I felt that would help.
>
>Taking the ref inside GetCurrentThread in my opinion is an incorrect
>change.   But I do not understand the ChangeLog entry nor why was that
>patch supposed to fix the other bug (or if the other bug listed on the
>SVN commit message is even the same fix).

As I understand it, the problem is that while certainly whenever a handle
is created, it must represent a reference to the underlying object, the
Windows GetCurrentThread() function doesn't actually create a handle. The
comment block above the function in io-layer/threads.c reflects this (and
it is, of course, documented in MSDN). If you create an actual handle but
you don't increment the underlying object's reference count, then when the
handle is closed, it'll decrement the reference count with no matching
increment and the object will end up being released early, before the last
handle is closed. What it boils down to is that it *is* correct behaviour
for GetCurrentThread to not increase the reference count, but it is not
correct behaviour for it to create a handle. Indeed, anyone following the
MSDN documentation will not even bother to pass the handle GetCurrentThread
returns into CloseHandle, so while in that situation, the object wouldn't
end up being freed early as the reference count decrement in CloseHandle
would never happen, a memory leak (albeit a small one) would occur (the
size of whatever sits between a HANDLE and the underlying object). This
leak is very likely what is causing the original poster's defect.

The log message at the quoted URL also confuses me; it looks as though
Gonzalo thinks io-layer's GetCurrentThread() is already matching
Microsoft's pseudohandle behaviour. Certainly, though, the patch is correct
if that code ever has to run on Windows (..and it'll also be correct
if/when mono starts using pseudohandles too). :-) 

Jonathan Gilbert