You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Bojan Smojver <bo...@rexursive.com> on 2006/03/27 23:40:41 UTC
apr_dbd_init
I asked a while back about having a test in this function to this effect:
if (drivers)
return APR_SUCCESS;
This was to make sure that more than one Apache module can call the
function without any ill effect. Obviously, calling this function from
more than one thread would still be unsafe, but that's not normally
the case within Apache, as most initialisation happens during process
creation (AFAIK).
I think Nick wanted to move this function into apr_initialize instead.
I wasn't sure if that was a good idea, because it may create APR
dependency on APU (which may already be the case - I don't know).
After that, I completely forgot about the issue, so we never reached a
conclusion on this and it was not addressed before the release. Sorry
:-(
What's the final verdict on this?
--
Bojan
Re: apr_dbd_init
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Bojan Smojver wrote:
>
> I think Nick wanted to move this function into apr_initialize instead.
> I wasn't sure if that was a good idea, because it may create APR
> dependency on APU (which may already be the case - I don't know).
That's a non-starter. apr is independent of apr-util.
Bill
Re: apr_dbd_init
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Nick Kew <ni...@webthing.com>:
> Proposed patch below.
One more thing I've forgotten in my previous e-mail. Unless there is
some kind of binary compatibility problem (I don't see it), this would
be a good candidate for backporting to 1.2.x.
--
Bojan
Re: apr_dbd_init
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sun, 2006-04-09 at 03:14 -0500, William A. Rowe, Jr. wrote:
> I'm rolling 5pm UT, and I'm sorry but no cycles and too little understanding
> of the dbd API for me to take this on :( Maybe another committer in the next
> 9 hours can take a peek?
No worries.
--
Bojan
Re: apr_dbd_init
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Bojan Smojver wrote:
> Quoting Bojan Smojver <bo...@rexursive.com>:
>
>> Any chance of a backport of this to 1.2.7?
I'm rolling 5pm UT, and I'm sorry but no cycles and too little understanding
of the dbd API for me to take this on :( Maybe another committer in the next
9 hours can take a peek?
Bill
Re: apr_dbd_init
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Bojan Smojver <bo...@rexursive.com>:
> Any chance of a backport of this to 1.2.7?
Just another ping...
--
Bojan
Re: apr_dbd_init
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Bojan Smojver <bo...@rexursive.com>:
> Any chance of a backport of this to 1.2.7?
Thanks (r392812 AFAICS)!
--
Bojan
Re: apr_dbd_init
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2006-03-31 at 11:00 +0100, Nick Kew wrote:
> --- apr_dbd.c (revision 390371)
> +++ apr_dbd.c (working copy)
> @@ -63,17 +63,32 @@
> }
> #endif
>
> +static apr_status_t apr_dbd_term(void *ptr)
> +{
> + /* set drivers to NULL so init can work again */
> + drivers = NULL;
> +
> + /* Everything else we need is handled by cleanups registered
> + * when we created mutexes and loaded DSOs
> + */
> + return APR_SUCCESS;
> +}
> +
> APU_DECLARE(apr_status_t) apr_dbd_init(apr_pool_t *pool)
> {
> apr_status_t ret = APR_SUCCESS;
> +
> + if (drivers != NULL) {
> + return APR_SUCCESS;
> + }
> drivers = apr_hash_make(pool);
> + apr_pool_cleanup_register(pool, NULL, apr_dbd_term,
> + apr_pool_cleanup_null);
>
>
> #if APR_HAS_THREADS
> ret = apr_thread_mutex_create(&mutex, APR_THREAD_MUTEX_DEFAULT, pool);
> - apr_pool_cleanup_register(pool, mutex,
> - CLEANUP_CAST apr_thread_mutex_destroy,
> - apr_pool_cleanup_null);
> + /* This already registers a pool cleanup */
> #endif
>
> #ifndef APR_DSO_BUILD
Any chance of a backport of this to 1.2.7?
--
Bojan
Re: apr_dbd_init
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Nick Kew <ni...@webthing.com>:
> Proposed patch below.
>
> [...snip...]
>
> --- apr_dbd.c (revision 390371)
> +++ apr_dbd.c (working copy)
> @@ -63,17 +63,32 @@
> }
> #endif
>
> +static apr_status_t apr_dbd_term(void *ptr)
> +{
> + /* set drivers to NULL so init can work again */
> + drivers = NULL;
> +
> + /* Everything else we need is handled by cleanups registered
> + * when we created mutexes and loaded DSOs
> + */
> + return APR_SUCCESS;
> +}
> +
> APU_DECLARE(apr_status_t) apr_dbd_init(apr_pool_t *pool)
> {
> apr_status_t ret = APR_SUCCESS;
> +
> + if (drivers != NULL) {
> + return APR_SUCCESS;
> + }
> drivers = apr_hash_make(pool);
> + apr_pool_cleanup_register(pool, NULL, apr_dbd_term,
> + apr_pool_cleanup_null);
>
>
> #if APR_HAS_THREADS
> ret = apr_thread_mutex_create(&mutex, APR_THREAD_MUTEX_DEFAULT, pool);
> - apr_pool_cleanup_register(pool, mutex,
> - CLEANUP_CAST apr_thread_mutex_destroy,
> - apr_pool_cleanup_null);
> + /* This already registers a pool cleanup */
> #endif
>
> #ifndef APR_DSO_BUILD
Looks good to me. I see you also removed the explicit mutex cleanup -
I never even noticed it was there before, but it was obviously a
segfault waiting to happen. Nice!
--
Bojan
Re: apr_dbd_init
Posted by Nick Kew <ni...@webthing.com>.
On Tuesday 28 March 2006 00:21, Bojan Smojver wrote:
> Quoting Nick Kew <ni...@webthing.com>:
> > I don't think your suggestion is quite enough, as the lifetime
> > of "drivers" is tied to the pool. But if we add a pool cleanup
> > that resets drivers to NULL, it should be fine AFAICS.
> >
> > I'll put that in if noone shouts.
>
> Excellent! Thanks.
Proposed patch below.
When I ran svn diff on it, I found another uncommitted change:
apr_dbd_get_name (get column names). I see a bunch of patches
from you dated February. I'm going to work through and (hopefully)
apply them now, if I can keep distractions at bay for long enough.
--- apr_dbd.c (revision 390371)
+++ apr_dbd.c (working copy)
@@ -63,17 +63,32 @@
}
#endif
+static apr_status_t apr_dbd_term(void *ptr)
+{
+ /* set drivers to NULL so init can work again */
+ drivers = NULL;
+
+ /* Everything else we need is handled by cleanups registered
+ * when we created mutexes and loaded DSOs
+ */
+ return APR_SUCCESS;
+}
+
APU_DECLARE(apr_status_t) apr_dbd_init(apr_pool_t *pool)
{
apr_status_t ret = APR_SUCCESS;
+
+ if (drivers != NULL) {
+ return APR_SUCCESS;
+ }
drivers = apr_hash_make(pool);
+ apr_pool_cleanup_register(pool, NULL, apr_dbd_term,
+ apr_pool_cleanup_null);
#if APR_HAS_THREADS
ret = apr_thread_mutex_create(&mutex, APR_THREAD_MUTEX_DEFAULT, pool);
- apr_pool_cleanup_register(pool, mutex,
- CLEANUP_CAST apr_thread_mutex_destroy,
- apr_pool_cleanup_null);
+ /* This already registers a pool cleanup */
#endif
#ifndef APR_DSO_BUILD
--
Nick Kew
Re: apr_dbd_init
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Nick Kew <ni...@webthing.com>:
> I don't think your suggestion is quite enough, as the lifetime
> of "drivers" is tied to the pool. But if we add a pool cleanup
> that resets drivers to NULL, it should be fine AFAICS.
>
> I'll put that in if noone shouts.
Excellent! Thanks.
--
Bojan
Re: apr_dbd_init
Posted by Nick Kew <ni...@webthing.com>.
On Monday 27 March 2006 22:40, Bojan Smojver wrote:
> I asked a while back about having a test in this function to this effect:
>
> if (drivers)
> return APR_SUCCESS;
>
> What's the final verdict on this?
I don't think there ever was a "final verdict".
I don't think your suggestion is quite enough, as the lifetime
of "drivers" is tied to the pool. But if we add a pool cleanup
that resets drivers to NULL, it should be fine AFAICS.
I'll put that in if noone shouts.
--
Nick Kew