On Wed, Sep 1, 2010 at 2:49 PM, dave b <db.pub.m...@gmail.com> wrote:
> On 1 September 2010 22:08, Jeff Trawick <traw...@gmail.com> wrote: > > On Wed, Sep 1, 2010 at 6:37 AM, Graham Dumpleton > > <graham.dumple...@gmail.com> wrote: > >> > >> On 1 September 2010 20:15, Graham Leggett <minf...@sharp.fm> wrote: > >> > On 01 Sep 2010, at 6:07 AM, dave b wrote: > >> > > >> >> What is the rational behind not checking the return value of > >> >> apr_palloc and apr_pcalloc? > >> > > >> > The rationale is to not be forced to check for and handle hundreds of > >> > potential failure cases when you're probably doomed anyway. > > Yes I agree :) > Hence, why the patch I put in the bug report used assert :) > > > >> > The APR pools API gives you the apr_pool_abort_set() function, which > >> > specifies a function to call if the memory allocation fails. In the > case > >> > of > >> > httpd, a function is registered which gracefully shuts down that > >> > particular > >> > server process if the allocation fails, and apr_palloc() is in the > >> > process > >> > guaranteed to never return NULL. > >> > >> Noting that apr_pool_abort_set() is only setup in Apache 2.3 and not > >> in Apache 2.2.16 or earlier. Not being in 2.X explains why I couldn't > >> find it before. > >> > >> Any reason why setting up apr_pool_abort_set() wasn't back ported to > >> Apache 2.2? > > > > dunno, but here's the commit and original discussion in case anyone wants > to > > read > > http://svn.apache.org/viewvc?view=revision&revision=406953 (dunno if it > was > > subsequently fixed) > > http://www.mail-archive.com/dev@httpd.apache.org/msg32257.html > > > Interesting patch, um but... I think there maybe a problem here: > > from apr :) > include/apr_pools.h > > > #if defined(DOXYGEN) > APR_DECLARE(apr_status_t) apr_pool_create(apr_pool_t **newpool, > apr_pool_t *parent); > #else > #if APR_POOL_DEBUG > #define apr_pool_create(newpool, parent) \ > apr_pool_create_ex_debug(newpool, parent, NULL, NULL, \ > APR_POOL__FILE_LINE__) > #else > #define apr_pool_create(newpool, parent) \ > apr_pool_create_ex(newpool, parent, NULL, NULL) > #endif > #endif > > And apr_pool_create_ex is: > > > APR_DECLARE(apr_status_t) apr_pool_create_ex(apr_pool_t **newpool, > apr_pool_t *parent, > apr_abortfunc_t abort_fn, > apr_allocator_t *allocator); > > > > First thing to note is that in main apr_app_initialize[1] in > server/main.c via init_process. When this happens the global_pool will > not have a abort_fn. *However*, cntx will (it is set :) ), > process->pconf will. > > Now we look in the actual code :) > See [0] > Right so if apr_pool_create gets called with null as the parent from > now on - there will be no default abort_fn. > Lets go hunting for this happens ;) see [2] > > Ok so lets pull out the scoreboard now ? > > /* ToDo: This function should be made to handle setting up > * a scoreboard shared between processes using any IPC technique, > * not just a shared memory segment > */ > static apr_status_t open_scoreboard(apr_pool_t *pconf) > { > #if APR_HAS_SHARED_MEMORY > apr_status_t rv; > char *fname = NULL; > apr_pool_t *global_pool; > > /* We don't want to have to recreate the scoreboard after > * restarts, so we'll create a global pool and never clean it. > */ > rv = apr_pool_create(&global_pool, NULL); > > > MMM I wonder where else httpd does these kind of things ;) > Remember my simple grep can *easily* miss some :) > My 2 cents: I doubt that any of the core devs are going to match you for devotion to this topic, but I'm sure we will review patches to trunk to fix somewhat practical scenarios, such as ensuring that memory allocation failures during request processing go through the common abort function, if the existing code doesn't already handle that. (OTOH, I wouldn't think many would find addressing un-aborted alloc failures during initialization so interesting.) That requires some investigation/understanding of the context of the code that passes NULL for the parent pool, which hopefully you can make an attempt at without dumping the grep output and selected source code to the mailing list ;) (I know that sounds rude, but its my best advice to pursuing that thread of investigation.) > Ok great just get the rest of world to use apache 2.3 which has had > this patch for over 4 years now. Just one problem: I don't see anyone > running apache 2.3 alpha 8 on their production servers. > > So perhaps apache 2.2 should get a backport of this 'fix' :-) > Apparently my fix that I suggested on the bug referenced in my > original email is also not going to work :) (it will not be enough :-) > ) > What's the big picture here from your standpoint? Why is 2.2 noticeably impaired without such changes? Are there really many users who need that abort function to tell them that httpd can't alloc more heap? Anyway, if 2.3 doesn't really have this suitably addressed it would be better to get that finished first. > > Also, note I hit that code path via apr testdir (in one of the tests > ;) ) - and I was not out of memory - I haven't checked which test it > is yet (it would be nice if the apr tests told me this...). > If the caller has hit that code path and they haven't defined an > abort_fn, I think it best to exit / abort for them - If a user wants > not to have that happen then they can just simply ensure they are > providing their own abort_fn :) > d...@apr.apache.org, but first try gdb or testall -v or anything else that you need to find exactly what/where the failure is
