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

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

[Ludovic Courtès]

Hi!

"Andy Wingo" <wingo@pobox.com> writes:

>     fix a couple leaks in ports.c.  thanks valgrind!
>     
>     * libguile/ports.c (scm_i_remove_port): Fix a case in which ports
>       explictly closed via close-port would leak their iconv_t data.
>       (scm_set_port_encoding_x): scm_i_set_port_encoding_x strdups its
>       argument, so we need to free the locale encoding of the incoming str.

Good catch!

> --- a/libguile/ports.c
> +++ b/libguile/ports.c
> @@ -661,6 +661,19 @@ scm_i_remove_port (SCM port)
>    scm_port_non_buffer (p);
>    p->putback_buf = NULL;
>    p->putback_buf_size = 0;
> +
> +  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.

Thanks,
Ludo’.

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

[Andy Wingo]

On Mon 21 Feb 2011 21:50, ludo@gnu.org (Ludovic Courtès) writes:

>> --- a/libguile/ports.c
>> +++ b/libguile/ports.c
>> @@ -661,6 +661,19 @@ scm_i_remove_port (SCM port)
>>    scm_port_non_buffer (p);
>>    p->putback_buf = NULL;
>>    p->putback_buf_size = 0;
>> +
>> +  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.

Regards,

Andy
-- 
http://wingolog.org/

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

[Ludovic Courtès]

Hello,

Andy Wingo <wingo@pobox.com> writes:

> On Mon 21 Feb 2011 21:50, ludo@gnu.org (Ludovic Courtès) writes:
>
>>> --- a/libguile/ports.c
>>> +++ b/libguile/ports.c
>>> @@ -661,6 +661,19 @@ scm_i_remove_port (SCM port)
>>>    scm_port_non_buffer (p);
>>>    p->putback_buf = NULL;
>>>    p->putback_buf_size = 0;
>>> +
>>> +  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!  ;-)

Ludo’.

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

[Andy Wingo]

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/

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

[Ludovic Courtès]

Hi!

Andy Wingo <wingo@pobox.com> writes:

> 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 ;)

Damn, indeed!

> 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 :)

Ouch.  Thanks for the explanation, I had clearly overlooked this.

Ludo’.