You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Nick Kew <ni...@webthing.com> on 2004/01/02 04:09:09 UTC
PATCH: apr_reslist_invalidate
Rationale: if an module gets a resource that proves to be bad (e.g.
a connection that's gone away), it shouldn't be returned to the
pool to be given out again. We should invalidate it.
I'm proposing the following patch, though I'm not sure whether
or not we should free_container in the event of destroy_resource
returning an error(?)
--- apr-util/include/apr_reslist.h.old 2004-01-02 02:46:38.000000000 +0000
+++ apr-util/include/apr_reslist.h.new 2004-01-02 02:50:43.000000000 +0000
@@ -150,6 +150,15 @@
APU_DECLARE(apr_status_t) apr_reslist_release(apr_reslist_t *reslist,
void *resource);
+/**
+ * Invalidate a resource in the pool - e.g. a database connection
+ * that returns a "lost connection" error and can't be restored.
+ * Use this instead of apr_reslist_release if the resource is bad.
+ */
+APU_DECLARE(apr_status_t) apr_reslist_invalidate(apr_reslist_t *reslist,
+ void *resource);
+
+
#ifdef __cplusplus
}
#endif
--- apr-util/misc/apr_reslist.c.old 2004-01-02 02:44:05.000000000 +0000
+++ apr-util/misc/apr_reslist.c.new 2004-01-02 02:51:27.000000000 +0000
@@ -396,5 +396,17 @@
return reslist_maint(reslist);
}
+APU_DECLARE(apr_status_t) apr_reslist_invalidate(apr_reslist_t *reslist,
+ apr_res_t *res)
+{
+ apr_thread_mutex_lock(reslist->listlock);
+ --reslist->ntotal ;
+ ret = destroy_resource(reslist, res) ;
+ if ( ret == APR_SUCCESS ) { /* what if it fails ??? */
+ free_container(reslist, res);
+ }
+ apr_thread_mutex_unlock(reslist->listlock);
+ return ret ;
+}
#endif /* APR_HAS_THREADS */
--
Nick Kew
Re: PATCH: apr_reslist_invalidate
Posted by Aaron Bannert <aa...@clove.org>.
On Mon, Mar 15, 2004 at 03:37:42AM +0000, Nick Kew wrote:
> It's 3 in the morning again, but I've written an updated patch,
> This time I've adapted the testsuite to watch it in action and
> successfully run it (Linux 2.4.x). As ever, more eyes better:-)
Tested and committed. Thanks for the patch, Nick.
-aaron
Re: PATCH: apr_reslist_invalidate
Posted by Nick Kew <ni...@webthing.com>.
On Sun, 14 Mar 2004, Jeff Trawick wrote:
> see comments in PR 26050 about discrepancy in prototypes that seems to show an
> underlying issue; apps don't see apr_res_t, so some rework and testing is needed
It's a fair cop guv. The resource is detached when it's issued, so
invalidate mustn't take it as attached. I must've posted a completely
wrong version from my experiments. Can't check, as the machine it's on
is out of action with dead hardware:-(
It's 3 in the morning again, but I've written an updated patch,
This time I've adapted the testsuite to watch it in action and
successfully run it (Linux 2.4.x). As ever, more eyes better:-)
--- testreslist.c.old 2004-03-15 02:04:21.000000000 +0000
+++ testreslist.c 2004-03-15 02:55:18.000000000 +0000
@@ -98,7 +98,8 @@
res = apr_palloc(pool, sizeof(*res));
res->id = my_params->c_count++;
- printf("++ constructing new resource [id:%d]\n", res->id);
+ printf("++ constructing new resource [id:%d, #%d/%d]\n", res->id,
+ my_params->c_count, my_params->d_count);
/* Sleep for awhile, to simulate construction overhead. */
apr_sleep(my_params->sleep_upon_construct);
@@ -114,8 +115,8 @@
my_resource_t *res = resource;
my_parameters_t *my_params = params;
- printf("-- destructing old resource [id:%d, #%d]\n", res->id,
- my_params->d_count++);
+ printf("-- destructing old resource [id:%d, #%d/%d]\n", res->id,
+ my_params->c_count, ++my_params->d_count);
apr_sleep(my_params->sleep_upon_destruct);
@@ -147,12 +148,23 @@
printf(" [tid:%d,iter:%d] using resource id:%d\n",
thread_info->tid,
i, res->id);
apr_sleep(thread_info->work_delay_sleep);
- rv = apr_reslist_release(rl, res);
- if (rv != APR_SUCCESS) {
- fprintf(stderr, "Failed to return resource to reslist\n");
- apr_thread_exit(thd, rv);
- return NULL;
- }
+/* simulate a 5% chance of the resource being bad */
+ if ( drand48() < 0.95 ) {
+ rv = apr_reslist_release(rl, res);
+ if (rv != APR_SUCCESS) {
+ fprintf(stderr, "Failed to return resource to reslist\n");
+ apr_thread_exit(thd, rv);
+ return NULL;
+ }
+ } else {
+ printf("invalidating resource id:%d\n", res->id) ;
+ rv = apr_reslist_invalidate(rl, res);
+ if (rv != APR_SUCCESS) {
+ fprintf(stderr, "Failed to invalidate resource\n");
+ apr_thread_exit(thd, rv);
+ return NULL;
+ }
+ }
}
return APR_SUCCESS;
@@ -167,6 +179,7 @@
int i;
apr_thread_t *my_threads[CONSUMER_THREADS];
my_thread_info_t my_thread_info[CONSUMER_THREADS];
+ srand48(time(0)) ;
printf("Creating child pool.......................");
rv = apr_pool_create(&pool, parpool);
--- apr_reslist.h.old 2004-03-15 01:21:22.000000000 +0000
+++ apr_reslist.h 2004-03-15 01:31:01.000000000 +0000
@@ -150,6 +150,15 @@
APU_DECLARE(apr_status_t) apr_reslist_release(apr_reslist_t *reslist,
void *resource);
+/**
+ * Invalidate a resource in the pool - e.g. a database connection
+ * that returns a "lost connection" error and can't be restored.
+ * Use this instead of apr_reslist_release if the resource is bad.
+ */
+APU_DECLARE(apr_status_t) apr_reslist_invalidate(apr_reslist_t *reslist,
+ void *resource);
+
+
#ifdef __cplusplus
}
#endif
--- apr_reslist.c.old 2004-03-15 01:22:05.000000000 +0000
+++ apr_reslist.c 2004-03-15 01:28:56.000000000 +0000
@@ -396,5 +396,15 @@
return reslist_maint(reslist);
}
+APU_DECLARE(apr_status_t) apr_reslist_invalidate(apr_reslist_t *reslist,
+ void *resource)
+{
+ apr_status_t ret ;
+ apr_thread_mutex_lock(reslist->listlock);
+ ret = reslist->destructor(resource, reslist->params, reslist->pool);
+ --reslist->ntotal ;
+ apr_thread_mutex_unlock(reslist->listlock);
+ return ret ;
+}
#endif /* APR_HAS_THREADS */
Re: PATCH: apr_reslist_invalidate
Posted by Jeff Trawick <tr...@attglobal.net>.
Nick Kew wrote:
> Rationale: if an module gets a resource that proves to be bad (e.g.
> a connection that's gone away), it shouldn't be returned to the
> pool to be given out again. We should invalidate it.
>
> I'm proposing the following patch, though I'm not sure whether
> or not we should free_container in the event of destroy_resource
> returning an error(?)
>
>
> --- apr-util/include/apr_reslist.h.old 2004-01-02 02:46:38.000000000 +0000
> +++ apr-util/include/apr_reslist.h.new 2004-01-02 02:50:43.000000000 +0000
> @@ -150,6 +150,15 @@
> APU_DECLARE(apr_status_t) apr_reslist_release(apr_reslist_t *reslist,
> void *resource);
>
> +/**
> + * Invalidate a resource in the pool - e.g. a database connection
> + * that returns a "lost connection" error and can't be restored.
> + * Use this instead of apr_reslist_release if the resource is bad.
> + */
> +APU_DECLARE(apr_status_t) apr_reslist_invalidate(apr_reslist_t *reslist,
> + void *resource);
> +
> +
> #ifdef __cplusplus
> }
> #endif
>
> --- apr-util/misc/apr_reslist.c.old 2004-01-02 02:44:05.000000000 +0000
> +++ apr-util/misc/apr_reslist.c.new 2004-01-02 02:51:27.000000000 +0000
> @@ -396,5 +396,17 @@
>
> return reslist_maint(reslist);
> }
> +APU_DECLARE(apr_status_t) apr_reslist_invalidate(apr_reslist_t *reslist,
> + apr_res_t *res)
see comments in PR 26050 about discrepancy in prototypes that seems to show an
underlying issue; apps don't see apr_res_t, so some rework and testing is needed
Re: PATCH: apr_reslist_invalidate
Posted by Jeff Trawick <tr...@attglobal.net>.
Aaron Bannert wrote:
> On Fri, Jan 02, 2004 at 03:09:09AM +0000, Nick Kew wrote:
>
>>Rationale: if an module gets a resource that proves to be bad (e.g.
>>a connection that's gone away), it shouldn't be returned to the
>>pool to be given out again. We should invalidate it.
>>
>>I'm proposing the following patch, though I'm not sure whether
>>or not we should free_container in the event of destroy_resource
>>returning an error(?)
>
>
> Did this ever get committed? I don't see it in HEAD, so it seems like
> it may have been forgotten.
not committed :(
> FWIW, I'm +1 on the concept, and I'd be glad to test/commit. We'll
> also want to add some test code while we're at it.
+1
Re: PATCH: apr_reslist_invalidate
Posted by Nick Kew <ni...@webthing.com>.
On Wed, 10 Mar 2004, Aaron Bannert wrote:
> On Fri, Jan 02, 2004 at 03:09:09AM +0000, Nick Kew wrote:
> > Rationale: if an module gets a resource that proves to be bad (e.g.
> > a connection that's gone away), it shouldn't be returned to the
> > pool to be given out again. We should invalidate it.
> >
> > I'm proposing the following patch, though I'm not sure whether
> > or not we should free_container in the event of destroy_resource
> > returning an error(?)
>
> Did this ever get committed? I don't see it in HEAD, so it seems like
> it may have been forgotten.
Nope, it got ignored[1], like everything else I've tried to contribute
to APR. OK that's not much, but then getting consistently ignored isn't
really an encouragement to share my work.
OK, I know exactly why that happens: we all have a limited supply of
round tuits, and a third-party patch is going to be in contention
with higher-priority demands on your time. Which leads me to wonder
if there could be a better way to submit proposed patches?
> FWIW, I'm +1 on the concept, and I'd be glad to test/commit.
Excellent! Thank you.
> We'll
> also want to add some test code while we're at it.
You mean to reslist in particular, or apr_util in general?
If the former, would it help for me to revisit it?
[1] Except for Paul Querna - who wants if for exactly the same
reasons I do but has no more privilege than I to commit.
--
Nick Kew
Re: PATCH: apr_reslist_invalidate
Posted by Aaron Bannert <aa...@clove.org>.
On Fri, Jan 02, 2004 at 03:09:09AM +0000, Nick Kew wrote:
> Rationale: if an module gets a resource that proves to be bad (e.g.
> a connection that's gone away), it shouldn't be returned to the
> pool to be given out again. We should invalidate it.
>
> I'm proposing the following patch, though I'm not sure whether
> or not we should free_container in the event of destroy_resource
> returning an error(?)
Did this ever get committed? I don't see it in HEAD, so it seems like
it may have been forgotten.
FWIW, I'm +1 on the concept, and I'd be glad to test/commit. We'll
also want to add some test code while we're at it.
-aaron
Re: PATCH: apr_reslist_invalidate
Posted by Paul Querna <ch...@force-elite.com>.
>>I'm proposing the following patch, though I'm not sure whether
>>or not we should free_container in the event of destroy_resource
>>returning an error(?)
I think that your patch has the correct behavior. If you look at the
reslist_cleanup function it will also not free the container if the
destroy_resource returns an error.
This is a very useful addition to resource lists, allowing things like
client sockets once invalid to be easily removed from the pool. It is
functionality that is needed as reslists usage increase. Can someone
please commit this patch?
-chip
Re: PATCH: apr_reslist_invalidate
Posted by Nick Kew <ni...@webthing.com>.
Grr ... I must not tidy up patches to send them immediately before
collapsing in bed at 3 a.m. Of course that's missing a declaration
of apr_status_t ret :-(
On Fri, 2 Jan 2004, Nick Kew wrote:
> Rationale: if an module gets a resource that proves to be bad (e.g.
> a connection that's gone away), it shouldn't be returned to the
> pool to be given out again. We should invalidate it.
>
> I'm proposing the following patch, though I'm not sure whether
> or not we should free_container in the event of destroy_resource
> returning an error(?)
>
>
--- apr-util/include/apr_reslist.h.old 2004-01-02 02:46:38.000000000 +0000
+++ apr-util/include/apr_reslist.h.new 2004-01-02 02:50:43.000000000 +0000
@@ -150,6 +150,15 @@
APU_DECLARE(apr_status_t) apr_reslist_release(apr_reslist_t *reslist,
void *resource);
+/**
+ * Invalidate a resource in the pool - e.g. a database connection
+ * that returns a "lost connection" error and can't be restored.
+ * Use this instead of apr_reslist_release if the resource is bad.
+ */
+APU_DECLARE(apr_status_t) apr_reslist_invalidate(apr_reslist_t *reslist,
+ void *resource);
+
+
#ifdef __cplusplus
}
#endif
--- apr-util/misc/apr_reslist.c.old 2004-01-02 02:44:05.000000000 +0000
+++ apr-util/misc/apr_reslist.c.new 2004-01-02 02:51:27.000000000 +0000
@@ -396,5 +396,17 @@
return reslist_maint(reslist);
}
+APU_DECLARE(apr_status_t) apr_reslist_invalidate(apr_reslist_t *reslist,
+ apr_res_t *res)
+{
+ apr_status_t ;
+ apr_thread_mutex_lock(reslist->listlock);
+ --reslist->ntotal ;
+ ret = destroy_resource(reslist, res) ;
+ if ( ret == APR_SUCCESS ) { /* what if it fails ??? */
+ free_container(reslist, res);
+ }
+ apr_thread_mutex_unlock(reslist->listlock);
+ return ret ;
+}
#endif /* APR_HAS_THREADS */
>
> --
> Nick Kew
>