Re: t/SMOKE points the finger at filter/out_bbs_filebucket.t

Joe Schaefer wrote:
> Stas Bekman <stas@xxxxxxxxxx> writes:
>
>
> > Joe Schaefer wrote:
> >
> >
> > > The problem is that the current modperl_bucket.c implementation
> > > needs a proper setaside function.  Think about it- the interpreter
> > > only hangs around for the request, but the bucket may be setaside
> > > in the core output filter for much longer than that.
> >
> > There is no problem per se, since the setaside slot is marked as:
> > apr_bucket_setaside_notimpl, so noone can try to setaside it.
>
>
> httpd-2.0's core_output_filter treats apr_bucket_setaside_notimpl
> as equivalent to apr_bucket_setaside_noop whenever it calls 
> ap_save_brigade.  It's probably a bug in httpd, but it's a 
> longstanding one. 

Oh, that's bad.

> In any case, it'd be good to implement setasides
> before the 2.0 release of modperl.

If that doesn't affect the user API, then it doesn't have to happen before 
the ever sliding release date. But if you work on that, that's cool.

> > As mentioned above, I don't think it's borked in it's current form. 
> > It's just incomplete. Though trying to complete it, may prove hard. 
>
>
> Well let's look at what needs completing:
>
> static const apr_bucket_type_t modperl_bucket_sv_type = {
>     "mod_perl SV bucket", 4,
> #if MODULE_MAGIC_NUMBER >= 20020602
>     APR_BUCKET_DATA,
> #endif
>     modperl_bucket_sv_destroy,
>     modperl_bucket_sv_read,
>     apr_bucket_setaside_notimpl,
>     apr_bucket_shared_split,
>     apr_bucket_shared_copy,
> };
>
>
> I'm of the opinion that none of the apr_bucket_shared_ functions
> belong there, because they allocate their buckets from a
> bucket allocator in bucket->list.  But that slot is never
> initialized in modperl_bucket.c - there are no apr_bucket_alloc_t *
> pointers even occuring in that file, so any calls to apr_bucket_alloc()
> would be invalid.  However apr_bucket_simple_copy() does just
> that, and it is invoked by both apr_bucket_shared_split() and 
> apr_bucket_shared_copy().
>
> So all three operations: 
>
>   setaside, split, and copy 
>
> currently need work, or else modperl_bucket_sv_create() needs
> to use apr_bucket_alloc/apr_bucket_free instead of malloc/free.

that's already scheduled for 2.0 release:
todo/release:

* consider changing the allocation method in APR::Bucket::new from
  malloc/free to bucket_alloc, like all other buckets do

If you can work on that, it'd be a great help, Joe.

> > On the other hand, it's very similar to pool cleanup handlers
> > implementation, where we store my_perl along with data, and use it to
> > work on the data later on. The concept should work just as well for
> > the setaside function implementation. 
>
>
> setasides for svbuckets should probably follow the transient bucket 
> implementation and convert the svbucket to a heap bucket.  I don't
> think it's particularly difficult.  The real question, I think, is whether
> or not we should add an apr_bucket_alloc_t * argument to 
> modperl_bucket_sv_create(), (so then APR::Bucket::new would
> expect another bucket allocator argument), or stick to malloc/free.

Well, if we didn't try to avoid the string copying, we could have just 
dropped modperl buckets altogether and just use plain string buckets. But 
it's not a good idea.

So hard is it going to be to stick with malloc/free vs. using the 
bucket_alloc. Or is there are a good reason against using malloc/free?


--
__________________________________________________________________
Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/     mod_perl Guide ---> http://perl.apache.org
mailto:stas@xxxxxxxxxx http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com