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.
It's the best to avoid internal knowledge, since those pieces of code then copied to the real-world situations where you can't be sure that those are modperl buckets.
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;
I like the idea of bb, but you will need to explicitly destroy it as well...
Someone has raised this point before, bb's should have a DESTROY method, it was trivial to add, but as soon as I did that, things started to crash, because of the bb's passed as an argument to input filters are special and can't be destoyed at the end of handler's scope (since the upstream filter won't see anything then). So if we code the filter handlers code to mark those bb's as special (so that they don't get destroyed at the end of the scope) then we can have the default APR::Brigade::DESTROY run automatically.
It might be a bit pedantic to worry this much about memory leaks in the test suite though.
Not pedantic at all. The test suite should be coded the correct way, since many of the techniques used in the tests are later picked up by real modules. Granted, some tests do wrong things on purpose, just to exercise how do we deal with wrong-doings.
-- __________________________________________________________________ Stas Bekman JAm_pH ------> Just Another mod_perl Hacker http://stason.org/ mod_perl Guide ---> http://perl.apache.org mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com http://modperlbook.org http://apache.org http://ticketmaster.com
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
