You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by Stas Bekman <st...@stason.org> on 2004/10/04 04:47:04 UTC
Re: cvs commit: modperl-2.0 Changes
joes@apache.org wrote:
> joes 2004/10/03 19:29:26
>
> Modified: . Changes
> Log:
> Reimplement APR::Bucket using apr_bucket_alloc_t -
>
> * $bucket_alloc argument added to APR::Bucket::new
>
> * new subs:
> APR::Bucket::setaside
Joe, there is a test where setaside is needed for real. It's:
t/filter/TestFilter/in_bbs_inject_header.pm: # it can be
stashed away (missing $b->setaside wrapper):
# XXX: this is broken: the bucket must be set-aside before
# it can be stashed away (missing $b->setaside wrapper)
push @{ $ctx->{buckets} }, $b;
debug "queued header [$data]";
inject_header_bucket($bb, $ctx);
next; # inject_header_bucket already called insert_tail
# notice that if we didn't inject any headers, this will
# still work ok, as inject_header_bucket will send the
# separator header which we just pushed to its queue
If you can deploy it there, that would be great.
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Stas Bekman <st...@stason.org>.
Joe Schaefer wrote:
> Stas Bekman <st...@stason.org> writes:
>
> [...]
>
>
>>Seriously, if we could abstract the magic you've added to the pool
>>code to be more general purpose (sans adding overhead by
>>generalization) it'd be nice to deploy it for $r, $c and may be other
>>objects...
>
>
> The solution for pools doesn't translate easily into a solution for
> other apache objects, because those objects don't have perl ctors
> like APR::Pool::new. So deploying something like it for $r and
> $c would mean putting it into those lightweight wrapper macros like
> mp_xs_Apache__RequestRec_2obj,
sure, that's doable
> which would certainly have a
> performance impact (especially if you're expecting to add a pool
> cleanup there).
The actual cleanup execution shouldn't be the problem, since it'll run
after the client has gone away. But registering should be.
Moreover there is a problem of PerlCleanupHandlers themselves, in case
they want to have $r available. At the moment there is no way to register
some cleanup as REALLY_REALLY_LAST
> AFIACT the only other candidate for doing something similar is
> APR__Brigade.h, but at the moment I still don't see a pressing
> need for it.
There is no need no, but I think it'd be great to do have it. Why? Because
it'd be nice to have idiomatic coding techniques not change after mp2 is
released. so people can just copy those snippets and just adjust them to
do the real work. So if you want to handle that, that would be great :)
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Stas Bekman <st...@stason.org> writes:
[...]
> Seriously, if we could abstract the magic you've added to the pool
> code to be more general purpose (sans adding overhead by
> generalization) it'd be nice to deploy it for $r, $c and may be other
> objects...
The solution for pools doesn't translate easily into a solution for
other apache objects, because those objects don't have perl ctors
like APR::Pool::new. So deploying something like it for $r and
$c would mean putting it into those lightweight wrapper macros like
mp_xs_Apache__RequestRec_2obj, which would certainly have a
performance impact (especially if you're expecting to add a pool
cleanup there).
AFIACT the only other candidate for doing something similar is
APR__Brigade.h, but at the moment I still don't see a pressing
need for it.
--
Joe Schaefer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Stas Bekman <st...@stason.org>.
Joe Schaefer wrote:
> Stas Bekman <st...@stason.org> writes:
>
>
>>Joe Schaefer wrote:
>>
>>>Stas Bekman <st...@stason.org> writes:
>>>[...]
>>>
>>>
>>>>I like the idea of bb, but you will need to explicitly destroy it as
>>>>well.. .
>>>
>>>Not sure I believe that...
>>
>>You mean all the calls to $bb->destroy are useless and even wrong?
>
>
> IMO they should probably be replaced with $bb->cleanup.
In which case there is no need to invoke new() inside the loop.
I suppose in some cases someone may still want to explicitly call destroy
to save memory, if they do a lot of allocations in a tight loop (but as
you say below it doesn't make any difference at the moment compared to
$bb->cleanup)
> Actually invoking
> $bb->destroy is almost always a mistake right now (even though win32
> reportedly has problems with that)...
Precisely, we can't use it until we figure out what the problem is.
> [...]
>
>
>>>I don't see the point of an APR::Brigade::DESTROY. What users need to know
>>>about brigades is
>>> 1) if they invoke $bb->destroy themselves, they become responsible
>>> for cleaning up any future buckets added to that brigade,
>>
>>Ah, shouldn't that be $bb->cleanup? If you destroy $bb, how can you
>>add any new buckets to it?
>
>
> Not quite. Right now all $bb->destroy does is run the pool cleanup that
> destroys its buckets, and then removes that pool cleanup. At one point
> the idea of allocating the brigade struct from the bucket allocator was
> tossed around, which would have made $bb->destroy wipe out the brigade
> struct as well (thus invalidating the C pointer). That change hasn't
> happened, yet, so the brigade struct is allocated from the pool.
OK, I wasn't aware of that, assuming that the docs are not declaring
something that the code doesn't do.
>>> 2) if they feed a brigade to ap_pass_brigade, according to the
>>> ap_pass_brigade docs, they should not touch that brigade object
>>> ever again. Even calling destroy() isn't documented as being
>>> safe.
>>
>>yes, that's certain.
>>
>>
>>>Trying to simplify some of this mess for modperl filter authors,
>>>perhaps with a smart DESTROY sub, will add lots of complexity to
>>>APR__Brigade.h - similar to what's in APR__Pool.h now.
>>
>>That's exactly what I was thinking about. Though I can see other
>>applications for your recent work on APR::Pool. For example we need
>>this kind of protection for $r and other major objects. If people get
>>$r into a closure, they have a guaranteed segfault on the next
>>request, which is usually very hard to trace. The SVMAGIC protection
>>will allow us to croak when someone writes a closure instead of segfault.
>
> That would be a nice thing to do.
I think we shouldn't bother loging it in todo lists, since I'm pretty sure
joes will post a patch in just a few minutes :)
Seriously, if we could abstract the magic you've added to the pool code to
be more general purpose (sans adding overhead by generalization) it'd be
nice to deploy it for $r, $c and may be other objects...
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Stas Bekman <st...@stason.org> writes:
> Joe Schaefer wrote:
> > Stas Bekman <st...@stason.org> writes:
> > [...]
> >
> > > I like the idea of bb, but you will need to explicitly destroy it as
> > > well.. .
> > Not sure I believe that...
>
> You mean all the calls to $bb->destroy are useless and even wrong?
IMO they should probably be replaced with $bb->cleanup. Actually invoking
$bb->destroy is almost always a mistake right now (even though win32
reportedly has problems with that)...
[...]
> > I don't see the point of an APR::Brigade::DESTROY. What users need to know
> > about brigades is
> > 1) if they invoke $bb->destroy themselves, they become responsible
> > for cleaning up any future buckets added to that brigade,
>
> Ah, shouldn't that be $bb->cleanup? If you destroy $bb, how can you
> add any new buckets to it?
Not quite. Right now all $bb->destroy does is run the pool cleanup that
destroys its buckets, and then removes that pool cleanup. At one point
the idea of allocating the brigade struct from the bucket allocator was
tossed around, which would have made $bb->destroy wipe out the brigade
struct as well (thus invalidating the C pointer). That change hasn't
happened, yet, so the brigade struct is allocated from the pool.
> > 2) if they feed a brigade to ap_pass_brigade, according to the
> > ap_pass_brigade docs, they should not touch that brigade object
> > ever again. Even calling destroy() isn't documented as being
> > safe.
>
> yes, that's certain.
>
> > Trying to simplify some of this mess for modperl filter authors,
> > perhaps with a smart DESTROY sub, will add lots of complexity to
> > APR__Brigade.h - similar to what's in APR__Pool.h now.
>
> That's exactly what I was thinking about. Though I can see other
> applications for your recent work on APR::Pool. For example we need
> this kind of protection for $r and other major objects. If people get
> $r into a closure, they have a guaranteed segfault on the next
> request, which is usually very hard to trace. The SVMAGIC protection
> will allow us to croak when someone writes a closure instead of segfault.
That would be a nice thing to do.
--
Joe Schaefer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Stas Bekman <st...@stason.org>.
Joe Schaefer wrote:
> Stas Bekman <st...@stason.org> writes:
>
> [...]
>
>
>>I like the idea of bb, but you will need to explicitly destroy it as well...
>
>
> Not sure I believe that...
You mean all the calls to $bb->destroy are useless and even wrong?
>>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.
>
>
> I don't see the point of an APR::Brigade::DESTROY. What users need to
> know about brigades is
>
> 1) if they invoke $bb->destroy themselves, they become responsible
> for cleaning up any future buckets added to that brigade,
Ah, shouldn't that be $bb->cleanup? If you destroy $bb, how can you add
any new buckets to it?
> 2) if they feed a brigade to ap_pass_brigade, according to the
> ap_pass_brigade docs, they should not touch that brigade object
> ever again. Even calling destroy() isn't documented as being
> safe.
yes, that's certain.
> Trying to simplify some of this mess for modperl filter authors,
> perhaps with a smart DESTROY sub, will add lots of complexity to
> APR__Brigade.h - similar to what's in APR__Pool.h now.
That's exactly what I was thinking about. Though I can see other
applications for your recent work on APR::Pool. For example we need this
kind of protection for $r and other major objects. If people get $r into a
closure, they have a guaranteed segfault on the next request, which is
usually very hard to trace. The SVMAGIC protection will allow us to croak
when someone writes a closure instead of segfault.
> I'm not
> sure it would be worthwhile to do that and risk introducing new
> bugs into the filter system. Also it's quite possible that
> dev@httpd will change its collective mind again about the
> correctness of (2), so things may get even messier if 2.0 and
> 2.2 have different ap_pass_brigade semantics.
Heh, who knows, by the time we get mp2.0 out, if Apache 2.2 is released,
we may just as well just go with 2.2 and drop 2.0 altogether :)
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Stas Bekman <st...@stason.org> writes:
[...]
> I like the idea of bb, but you will need to explicitly destroy it as well...
Not sure I believe that...
> 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.
I don't see the point of an APR::Brigade::DESTROY. What users need to
know about brigades is
1) if they invoke $bb->destroy themselves, they become responsible
for cleaning up any future buckets added to that brigade,
2) if they feed a brigade to ap_pass_brigade, according to the
ap_pass_brigade docs, they should not touch that brigade object
ever again. Even calling destroy() isn't documented as being
safe.
Trying to simplify some of this mess for modperl filter authors,
perhaps with a smart DESTROY sub, will add lots of complexity to
APR__Brigade.h - similar to what's in APR__Pool.h now. I'm not
sure it would be worthwhile to do that and risk introducing new
bugs into the filter system. Also it's quite possible that
dev@httpd will change its collective mind again about the
correctness of (2), so things may get even messier if 2.0 and
2.2 have different ap_pass_brigade semantics.
--
Joe Schaefer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Stas Bekman <st...@stason.org>.
Joe Schaefer wrote:
> Stas Bekman <st...@stason.org> 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:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Stas Bekman <st...@stason.org> 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: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Stas Bekman <st...@stason.org>.
Joe Schaefer wrote:
> Stas Bekman <st...@stason.org> writes:
>
> [...]
>
>
>>Joe, there is a test where setaside is needed for real. It's:
>>
>>t/filter/TestFilter/in_bbs_inject_header.pm: # it can be stashed
>>away (missing $b->setaside wrapper):
>>
>> # XXX: this is broken: the bucket must be set-aside before
>> # it can be stashed away (missing $b->setaside wrapper)
>> push @{ $ctx->{buckets} }, $b;
>> debug "queued header [$data]";
>> inject_header_bucket($bb, $ctx);
>> next; # inject_header_bucket already called insert_tail
>> # notice that if we didn't inject any headers, this will
>> # still work ok, as inject_header_bucket will send the
>> # separator header which we just pushed to its queue
>>
>>If you can deploy it there, that would be great.
>
>
> No problem. Is this what you have in mind?
>
> Index: t/filter/TestFilter/in_bbs_inject_header.pm
> ===================================================================
> RCS file: /home/cvs/modperl-2.0/t/filter/TestFilter/in_bbs_inject_header.pm,v
> retrieving revision 1.12
> diff -u -r1.12 in_bbs_inject_header.pm
> --- t/filter/TestFilter/in_bbs_inject_header.pm 4 Oct 2004 02:16:42 -0000 1.12
> +++ t/filter/TestFilter/in_bbs_inject_header.pm 4 Oct 2004 03:30:12 -0000
> @@ -179,7 +179,9 @@
> if ($data and $data =~ /^POST/) {
> # demonstrate how to add a header while processing other headers
> my $header = "$header1_key: $header1_val\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]";
> }
> 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?
> 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?
In the real module
(http://search.cpan.org/dist/Apache-Filter-HTTPHeadersFixup/) I don't use
bucket brigades to do the storage, but only strings. But I've left this
test to use buckets so we have more tests (which needs to be polished).
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0 Changes
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Stas Bekman <st...@stason.org> writes:
[...]
> Joe, there is a test where setaside is needed for real. It's:
>
> t/filter/TestFilter/in_bbs_inject_header.pm: # it can be stashed
> away (missing $b->setaside wrapper):
>
> # XXX: this is broken: the bucket must be set-aside before
> # it can be stashed away (missing $b->setaside wrapper)
> push @{ $ctx->{buckets} }, $b;
> debug "queued header [$data]";
> inject_header_bucket($bb, $ctx);
> next; # inject_header_bucket already called insert_tail
> # notice that if we didn't inject any headers, this will
> # still work ok, as inject_header_bucket will send the
> # separator header which we just pushed to its queue
>
> If you can deploy it there, that would be great.
No problem. Is this what you have in mind?
Index: t/filter/TestFilter/in_bbs_inject_header.pm
===================================================================
RCS file: /home/cvs/modperl-2.0/t/filter/TestFilter/in_bbs_inject_header.pm,v
retrieving revision 1.12
diff -u -r1.12 in_bbs_inject_header.pm
--- t/filter/TestFilter/in_bbs_inject_header.pm 4 Oct 2004 02:16:42 -0000 1.12
+++ t/filter/TestFilter/in_bbs_inject_header.pm 4 Oct 2004 03:30:12 -0000
@@ -179,7 +179,9 @@
if ($data and $data =~ /^POST/) {
# demonstrate how to add a header while processing other headers
my $header = "$header1_key: $header1_val\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]";
}
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);
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()).
--
Joe Schaefer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org