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]
