Re: Changing the order of cleanup for some core objects

[Joe Orton]

On Mon, Jul 21, 2008 at 10:11:44AM -0700, Chris Darroch wrote:
>   The particular issue I'd like to see addressed with any kind of
> "pre-cleanup" of APR pools is the one we work around today in mod_dbd.
> That's a concrete example of a difficult hack around the behaviour
> of APR pools and reslists.

Thanks a lot for writing that up, that's very useful to see.

After my initial analysis of that problem I was ready to agree that the 
pre-cleanups seem like an attractive and appropriate solution.  But 
since working out how I would solve the problem *without* using 
pre-cleanups, I am no longer convinced.

Here's how I would do it:

0. create a subpool for the reslist as a whole, P 
1. create a subpool Qn of P for each resource Rn in the reslist code, 
and pass that Qn to the constructor and destructor; store a reference 
to Qn in the apr_res_t for Rn
2. register the resource destructor as a cleanup on Qn, for the
corresponding resource Rn
3. destroy_resource(Rn) just destroys the Qn subpool
4. reslist_cleanup() no longer calls destroy_resource() on each 
resource; it can rely on subpool destruction to do that
5. reslist_cleanup() is registered against P rather than the 
passed-in pool
6. apr_reslist_destroy() just destroys P

I can't see any cleanup ordering problems with that scheme.

This does mandate use of a subpool for each resource, but presumably 
that is mandatory anyway; any resource constructor implementation which 
used the main pool would be leaky.

I think that would simplify and improve the code overall both in 
APR-util and in the reslist API consumers:

- consumers no longer have to manage subpools; simpler consumers
- not possible (easy) to write leaky consumers which forget to
create a subpool; correctness by design
- slightly simpler apr_reslist.c, maybe

whereas adding pre-cleanups has just *increased* complexity of APR as a 
whole, so far as I can see.

There is a design pattern (if you can call it that) in APR which is 
important to maintain: object destructors must be implemented as, or 
wrapped in, a pool cleanup, and must only ever be invoked by running 
that cleanup.  You see that in most of the APR code.

Avoiding that "design pattern" will lead to this conflict between 
implicit (pool-destroy driven) and explicit (apr_reslist_* API-driven) 
destruction, which is what you've got here with the reslist code and 
possibly what Mladen is seeing also.

Regards, Joe