Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.0-4-g3e05fc0

[Andy Wingo <wingo@pobox.com>]

On Tue 22 Feb 2011 12:36, ludo@gnu.org (Ludovic Courtès) writes:

>>>> +
>>>> +  if (p->input_cd != (iconv_t) -1)
>>>> +    {
>>>> +      iconv_close (p->input_cd);
>>>> +      p->input_cd = (iconv_t) -1;
>>>> +    }
>>>> +  
>>>> +  if (p->output_cd != (iconv_t) -1)
>>>> +    {
>>>> +      iconv_close (p->output_cd);
>>>> +      p->output_cd = (iconv_t) -1;
>>>> +    }
>>>> +
>>>
>>> I don’t think this is needed: each port has a finalizer,
>>> ‘finalize_port’, which normally takes care of this, eventually.
>>
>> It is needed, but only in the case that you `close-port' explicitly.
>> The block in finalize_port only takes care of gc'd open ports.
>
> Right.  Closed ports are eventually GC’d, so in that sense it is not
> strictly needed, but OK.
>
> Valgrind was wrong!  ;-)

You are setting yourself up for a fall here ;)

When you close a port via "close-port", you remove the port's
SCM_PTAB_ENTRY (port).  The SCM_PTAB_ENTRY points to the iconv_t, so the
finalizer would not have a chance to free it, because it can't get to
it.  You need to free the iconv_t at the time you remove the link from
the port to the SCM_PTAB_ENTRY -- i.e. at close-port time.  Otherwise
you leak the iconv_t.

Trust me: about 500K requests into meta/guile examples/web/hello.scm,
the memory usage was up at about a gigabyte or so :)

Andy
-- 
http://wingolog.org/