Stas Bekman <[EMAIL PROTECTED]> writes:

[...]

> > -            push @{ $ctx->{buckets} }, APR::Bucket->new($c->bucket_alloc, 
> > $header);
> > +            my $bucket = APR::Bucket->new($c->bucket_alloc, $header);
> > +            $bucket->setaside($c->pool);
> > +            push @{ $ctx->{buckets} }, $bucket;
> >              debug "queued header [$header]";
> >          }
> >          elsif ($data =~ /^[\r\n]+$/) {
> > @@ -197,7 +199,9 @@
> >              # time to add extra headers:
> >              for my $key (keys %headers) {
> >                  my $header = "$key: $headers{$key}\n";
> > -                push @{ $ctx->{buckets} }, APR::Bucket->new($c->bucket_alloc, 
> > $header);
> > +                my $bucket =  APR::Bucket->new($c->bucket_alloc, $header);
> > +                $bucket->setaside($c->pool);
> > +                push @{ $ctx->{buckets} }, $bucket;
> >                  debug "queued header [$header]";
> >              }
> > @@ -205,8 +209,8 @@
> >              # the separator header will be sent as a last header
> >              # so we send one newly added header and push the separator
> >              # to the end of the queue
> > -            # XXX: this is broken: the bucket must be set-aside before
> > -            # it can be stashed away (missing $b->setaside wrapper)
> > +
> > +            $b->setaside($c->pool);
> >              push @{ $ctx->{buckets} }, $b;
> >              debug "queued header [$data]";
> >              inject_header_bucket($bb, $ctx);
> 
> I think so. It was obviously wrong before, isn't it?

Yes, the $b->setaside($c->pool) call is certainly necessary.  The earlier
setaside calls I added aren't really required, because those modperl 
buckets already belong to you, so you know what their lifetime is.

> 
> > One criticism I have about this test- it is technically
> > incorrect to store the setaside buckets in a Perl array
> > instead of keeping them in a bucket brigade.  That's because any buckets
> > left-over in the array will not get destroyed,
> > which can cause a memory leak.  It is always good practice to always keep
> > buckets in brigades, because the brigade's cleanup should take care of this
> > (which is the reason why calling destroy() on a brigade is inferior to
> > calling cleanup()).
> 
> An excellent point, Joe. So should we just change the AV for bb and
> store that in the context?

Perhaps, or just make the array-ref an object with an appropriate
DESTROY sub, that will traverse its elements with 

  $_->destroy for @bucket_array;

It might be a bit pedantic to worry this much about memory leaks in the 
test suite though.
-- 
Joe Schaefer


---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to