You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Bill Stoddard <bi...@wstoddard.com> on 2001/08/03 02:17:54 UTC
apr_bucket_destroy & APR memory management
apr_bucket_destroy is defined in apr_buckets.h as follows...
#define apr_bucket_destroy(e) do { \
e->type->destroy(e->data); \
free(e); \
} while(0)
The problem is the presence of free(e); specifically, freeing the bucket directly in this
macro ties -all- buckets, even custom user buckets, to the same memory management scheme
(CRT in this case). Folks that want to write custom buckets may not want to use APRs
default bucket memory management.
Saeid Sakhitab in the IBM AS/400 team pointed this out to me and has a couple of
suggestions on how to fix this.
Fix 1...
Pass the entire bucket to the destroy function and let destroy free the bucket right
before return.
#define apr_bucket_destroy(e) do { \
e->type->destroy(e); \ /* e is freed in destroy() */
} while(0)
One irritation with this method is that the destroy() function is called internal in many
of the bucket functions (search for file_destroy() in apr_bucket_file to see what I mean).
Fix 2
Define a new field in the generic bucket structure, a pointer to a function to free the
bucket. For example:
#define apr_bucket_destroy(e) do { \
e->type->destroy(e->data); \
e->type->free(e); \
} while(0)
This requires each bucket implement a free() function.
Any strong opinions on which way to go (or suggestions for an entirely different
solution)? I'll implement whichever one the group decides on.
Bill
Re: apr_bucket_destroy & APR memory management
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 7 Aug 2001, Roy T. Fielding wrote:
> The short answer is: don't morph buckets. The long answer is that
> free is a function of how the bucket content was allocated and not a
> function of its behavior as a bucket type. So either move free out of
> apr_bucket_type_t
That makes sense. So to restate my understanding of what you just said,
apr_bucket_type_t is for functions that operate on the data. apr_bucket
function pointers (of which there are currently none) operate on the
bucket itself. free() should be one of the latter.
+1.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_bucket_destroy & APR memory management
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
> > I would think that the bucket memory allocator and the bucket memory
> > deallocator would have to be consistent. Put it in the pool.
>
> Huh? What pool?
The pool that would need to be in the bucket structure if you want to support
morphing of buckets. Never mind.
The short answer is: don't morph buckets. The long answer is that free is
a function of how the bucket content was allocated and not a function of
its behavior as a bucket type. So either move free out of apr_bucket_type_t
or compound the number of bucket types by the number of potential free
mechanisms for those types.
....Roy
Re: apr_bucket_destroy & APR memory management
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 7 Aug 2001, Roy T. Fielding wrote:
> It only imposes the restriction that a routine cannot morph a bucket
> without also morphing the contents of that bucket, which in some cases
> means transferring the data and freeing the old allocation.
You have to use the original apr_bucket struct for the new morphed bucket
because you mess up the brigade otherwise, and even if you tried to
account for that, you'd still probably invalidate bucket pointers in the
caller.
> I would think that the bucket memory allocator and the bucket memory
> deallocator would have to be consistent. Put it in the pool.
Huh? What pool?
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_bucket_destroy & APR memory management
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
On Sat, Aug 04, 2001 at 12:14:28AM -0400, Cliff Woolley wrote:
> On Fri, 3 Aug 2001, Roy T. Fielding wrote:
>
> > If you can morph a bucket type to another bucket type, then you already
> > know what the two types are and hence whether or not they will have the
> > same free function. Otherwise, the act of morphing is already broken,
> > since it is already making a lot more assumptions than allocation source.
>
> There's still a problem with that, though. Take this typical situation:
>
> e = apr_bucket_foo_create();
> apr_bucket_read(e);
> ... morphs to type bar
> apr_bucket_destroy(e);
> ... calls ->type->free() pointer from bar, not foo
>
> This imposes a lot of potentially nasty restrictions: (a) the original foo
> apr_bucket must have been allocated with the same type as a bar apr_bucket
> would be allocated with, (b) foo buckets can never support morphing to
> both bar1 buckets and bar2 buckets simultaneously if bar1 and bar2 use
> different allocators, (c, which follows from a and b) neither foo nor bar
> buckets, which have the same, non-standard allocator due to restriction
> (a), can ever be morphed to a standard bucket type (eg heap, which is a
> staple of morphing), due to restriction (b).
It only imposes the restriction that a routine cannot morph a bucket
without also morphing the contents of that bucket, which in some cases
means transferring the data and freeing the old allocation.
> The only way out of this dilemma that I can think of I hate, which is to
> put the ->free pointer in the apr_bucket rather than in the
> apr_bucket_type_t. That sucks. If you've got a better idea, I'm all
> ears.
I would think that the bucket memory allocator and the bucket memory
deallocator would have to be consistent. Put it in the pool.
....Roy
Re: apr_bucket_destroy & APR memory management
Posted by Cliff Woolley <cl...@yahoo.com>.
On Fri, 3 Aug 2001, Roy T. Fielding wrote:
> If you can morph a bucket type to another bucket type, then you already
> know what the two types are and hence whether or not they will have the
> same free function. Otherwise, the act of morphing is already broken,
> since it is already making a lot more assumptions than allocation source.
There's still a problem with that, though. Take this typical situation:
e = apr_bucket_foo_create();
apr_bucket_read(e);
... morphs to type bar
apr_bucket_destroy(e);
... calls ->type->free() pointer from bar, not foo
This imposes a lot of potentially nasty restrictions: (a) the original foo
apr_bucket must have been allocated with the same type as a bar apr_bucket
would be allocated with, (b) foo buckets can never support morphing to
both bar1 buckets and bar2 buckets simultaneously if bar1 and bar2 use
different allocators, (c, which follows from a and b) neither foo nor bar
buckets, which have the same, non-standard allocator due to restriction
(a), can ever be morphed to a standard bucket type (eg heap, which is a
staple of morphing), due to restriction (b).
The only way out of this dilemma that I can think of I hate, which is to
put the ->free pointer in the apr_bucket rather than in the
apr_bucket_type_t. That sucks. If you've got a better idea, I'm all
ears.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_bucket_destroy & APR memory management
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
> The problem is simple: until we have SMS, all apr_bucket structs _must_ be
> allocated with the same allocator (malloc) and therefore freed with the
> same function (free). Why? Because the apr_bucket struct is not type
> specific and actually frequently changes types. Any time we morph from
> one bucket type to the next, we take an apr_bucket that was allocated by
> one type's _create() function and just reset its ->type and ->data
> pointers. If each type gets its own free function, we might have an
> apr_bucket that was allocated by one bucket type using one allocator that
> gets morphed to another bucket type. Calling free on that apr_bucket is
> now an invalid function. Working around this problem without SMS requires
> inserting a lot of special cases anytime we attempt to morph. How does
> SMS help? Because with SMS, each apr_bucket struct will get an ->sms
> pointer that indicates what SMS was used to allocate that bucket (so that
> the private structures for the bucket can be allocated in the same SMS).
> THAT's the sms to use when freeing that apr_bucket, and it won't change
> if/when we morph that bucket to a new type.
>
> Unless you can think of a clean way around this problem, I think we need
> to back out this patch.
If you can morph a bucket type to another bucket type, then you already
know what the two types are and hence whether or not they will have the
same free function. Otherwise, the act of morphing is already broken,
since it is already making a lot more assumptions than allocation source.
....Roy
Re: apr_bucket_destroy & APR memory management
Posted by Cliff Woolley <cl...@yahoo.com>.
On Fri, 3 Aug 2001, Bill Stoddard wrote:
> > +0.9 It should be really easy, in fact... just stick free itself in for
> > that function pointer in all currently implemented bucket types.
>
> Okay. If I hear no dissents, I'll do it tomorrow.
I was in the process of cleaning up some of the comments in apr_buckets.h
about the various function pointers and what they do, and I thought of a
big big problem with this. I have to negate my vote to -0.9. :-(
The problem is simple: until we have SMS, all apr_bucket structs _must_ be
allocated with the same allocator (malloc) and therefore freed with the
same function (free). Why? Because the apr_bucket struct is not type
specific and actually frequently changes types. Any time we morph from
one bucket type to the next, we take an apr_bucket that was allocated by
one type's _create() function and just reset its ->type and ->data
pointers. If each type gets its own free function, we might have an
apr_bucket that was allocated by one bucket type using one allocator that
gets morphed to another bucket type. Calling free on that apr_bucket is
now an invalid function. Working around this problem without SMS requires
inserting a lot of special cases anytime we attempt to morph. How does
SMS help? Because with SMS, each apr_bucket struct will get an ->sms
pointer that indicates what SMS was used to allocate that bucket (so that
the private structures for the bucket can be allocated in the same SMS).
THAT's the sms to use when freeing that apr_bucket, and it won't change
if/when we morph that bucket to a new type.
Unless you can think of a clean way around this problem, I think we need
to back out this patch.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_bucket_destroy & APR memory management
Posted by Bill Stoddard <bi...@wstoddard.com>.
> On Thu, 2 Aug 2001, Bill Stoddard wrote:
>
> > Fix 1... Pass the entire bucket to the destroy function and let
> > destroy free the bucket right before return.
> > #define apr_bucket_destroy(e) do { \
> > e->type->destroy(e); \ /* e is freed in destroy() */
> > } while(0)
> >
> > One irritation with this method is that the destroy() function is
> > called internal in many of the bucket functions (search for
> > file_destroy() in apr_bucket_file to see what I mean).
>
> That's the whole problem, yeah. -1.
>
>
> > Fix 2 Define a new field in the generic bucket structure, a pointer to
> > a function to free the bucket. For example:
> > #define apr_bucket_destroy(e) do { \
> > e->type->destroy(e->data); \
> > e->type->free(e); \
> > } while(0)
> >
> > This requires each bucket implement a free() function.
> >
> > Any strong opinions on which way to go (or suggestions for an entirely
> > different solution)? I'll implement whichever one the group decides
> > on.
>
> +0.9 It should be really easy, in fact... just stick free itself in for
> that function pointer in all currently implemented bucket types.
Okay. If I hear no dissents, I'll do it tomorrow.
>
> However: once we switch buckets to use SMS, none of this will really be
> necessary... s/free/apr_sms_free/ in the apr_bucket_destroy() macro, and
> then SMS handles the polymorphism for you. Just a thought.
Quite interesting! Still, I think it makes sense to have bucket specific free functions.
This is exactly analogous to a class destructor, which lives inside, not outside, the
class.
Bill
Re: apr_bucket_destroy & APR memory management
Posted by Cliff Woolley <cl...@yahoo.com>.
On Thu, 2 Aug 2001, Bill Stoddard wrote:
> Fix 1... Pass the entire bucket to the destroy function and let
> destroy free the bucket right before return.
> #define apr_bucket_destroy(e) do { \
> e->type->destroy(e); \ /* e is freed in destroy() */
> } while(0)
>
> One irritation with this method is that the destroy() function is
> called internal in many of the bucket functions (search for
> file_destroy() in apr_bucket_file to see what I mean).
That's the whole problem, yeah. -1.
> Fix 2 Define a new field in the generic bucket structure, a pointer to
> a function to free the bucket. For example:
> #define apr_bucket_destroy(e) do { \
> e->type->destroy(e->data); \
> e->type->free(e); \
> } while(0)
>
> This requires each bucket implement a free() function.
>
> Any strong opinions on which way to go (or suggestions for an entirely
> different solution)? I'll implement whichever one the group decides
> on.
+0.9 It should be really easy, in fact... just stick free itself in for
that function pointer in all currently implemented bucket types. No need
to implement anything.
However: once we switch buckets to use SMS, none of this will really be
necessary... s/free/apr_sms_free/ in the apr_bucket_destroy() macro, and
then SMS handles the polymorphism for you. Just a thought.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA