You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Joe Orton <jo...@redhat.com> on 2008/11/25 14:09:21 UTC
Re: svn commit: r719505 - in /apr/apr-util/branches/1.3.x:
Makefile.in build.conf build/dbm.m4 build/dso.m4 dbm/apr_dbm.c
dbm/apr_dbm_berkeleydb.c dbm/apr_dbm_gdbm.c dbm/apr_dbm_ndbm.c
include/private/apr_dbm_private.h
On Fri, Nov 21, 2008 at 06:32:58AM -0000, William Rowe wrote:
> Author: wrowe
> Date: Thu Nov 20 22:32:58 2008
> New Revision: 719505
>
> URL: http://svn.apache.org/viewvc?rev=719505&view=rev
> Log:
> Introduce DBM DSO linkage.
>
> Backports: 719504
...
> Modified: apr/apr-util/branches/1.3.x/dbm/apr_dbm.c
> URL: http://svn.apache.org/viewvc/apr/apr-util/branches/1.3.x/dbm/apr_dbm.c?rev=719505&r1=719504&r2=719505&view=diff
> ==============================================================================
> --- apr/apr-util/branches/1.3.x/dbm/apr_dbm.c (original)
> +++ apr/apr-util/branches/1.3.x/dbm/apr_dbm.c Thu Nov 20 22:32:58 2008
...
> +static apr_status_t dbm_open_type(apr_dbm_type_t const* * vtable,
> + const char *type,
> + apr_pool_t *pool)
> +{
...
> +
> + if (!drivers)
> + {
> + apr_pool_t *ppool = pool;
> + apr_pool_t *parent;
> +
> + /* Top level pool scope, need process-scope lifetime */
> + for (parent = pool; parent; parent = apr_pool_parent_get(ppool))
> + ppool = parent;
> +
> + /* deprecate in 2.0 - permit implicit initialization */
> + apu_dso_init(ppool);
> +
> + drivers = apr_hash_make(ppool);
> + apr_hash_set(drivers, "sdbm", APR_HASH_KEY_STRING, &apr_dbm_type_sdbm);
This surely isn't thread-safe? Two threads could enter apr_dbm_open*()
with drivers == NULL and then call apu_dso_init() on the same global
pool, which would break.
joe
Re: svn commit: r719505 - in /apr/apr-util/branches/1.3.x: Makefile.in
build.conf build/dbm.m4 build/dso.m4 dbm/apr_dbm.c dbm/apr_dbm_berkeleydb.c
dbm/apr_dbm_gdbm.c dbm/apr_dbm_ndbm.c include/private/apr_dbm_private.h
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
>
> apu_dso_init() and apr_hash_make() both result in calls to palloc for
> ppool - that's what isn't thread-safe.
Gotcha, I haven't forgotten your point, I'm only still coming up with
some workaround. Thanks for the observation.
Re: svn commit: r719505 - in /apr/apr-util/branches/1.3.x:
Makefile.in build.conf build/dbm.m4 build/dso.m4 dbm/apr_dbm.c
dbm/apr_dbm_berkeleydb.c dbm/apr_dbm_gdbm.c dbm/apr_dbm_ndbm.c
include/private/apr_dbm_private.h
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Nov 25, 2008 at 12:50:08PM -0600, William Rowe wrote:
> Joe Orton wrote:
> > On Fri, Nov 21, 2008 at 06:32:58AM -0000, William Rowe wrote:
> >> Author: wrowe
> >> Date: Thu Nov 20 22:32:58 2008
> >> New Revision: 719505
> >>
> >> URL: http://svn.apache.org/viewvc?rev=719505&view=rev
> >> Log:
> >> Introduce DBM DSO linkage.
> >>
> >> Backports: 719504
> > ...
> >> Modified: apr/apr-util/branches/1.3.x/dbm/apr_dbm.c
> >> URL: http://svn.apache.org/viewvc/apr/apr-util/branches/1.3.x/dbm/apr_dbm.c?rev=719505&r1=719504&r2=719505&view=diff
> >> ==============================================================================
> >> --- apr/apr-util/branches/1.3.x/dbm/apr_dbm.c (original)
> >> +++ apr/apr-util/branches/1.3.x/dbm/apr_dbm.c Thu Nov 20 22:32:58 2008
> > ...
> >> +static apr_status_t dbm_open_type(apr_dbm_type_t const* * vtable,
> >> + const char *type,
> >> + apr_pool_t *pool)
> >> +{
> > ...
> >> +
> >> + if (!drivers)
> >> + {
> >> + apr_pool_t *ppool = pool;
> >> + apr_pool_t *parent;
> >> +
> >> + /* Top level pool scope, need process-scope lifetime */
> >> + for (parent = pool; parent; parent = apr_pool_parent_get(ppool))
> >> + ppool = parent;
> >> +
> >> + /* deprecate in 2.0 - permit implicit initialization */
> >> + apu_dso_init(ppool);
> >> +
> >> + drivers = apr_hash_make(ppool);
> >> + apr_hash_set(drivers, "sdbm", APR_HASH_KEY_STRING, &apr_dbm_type_sdbm);
> >
> > This surely isn't thread-safe? Two threads could enter apr_dbm_open*()
> > with drivers == NULL and then call apu_dso_init() on the same global
> > pool, which would break.
...
> If nobody has performed any database, sql, ldap or other access prior to
> thread creation and race-for-first, then you are right. It won't break,
> but it certainly can cause a double-load artifact due to the race.
apu_dso_init() and apr_hash_make() both result in calls to palloc for
ppool - that's what isn't thread-safe.
joe
Re: svn commit: r719505 - in /apr/apr-util/branches/1.3.x: Makefile.in
build.conf build/dbm.m4 build/dso.m4 dbm/apr_dbm.c dbm/apr_dbm_berkeleydb.c
dbm/apr_dbm_gdbm.c dbm/apr_dbm_ndbm.c include/private/apr_dbm_private.h
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Fri, Nov 21, 2008 at 06:32:58AM -0000, William Rowe wrote:
>> Author: wrowe
>> Date: Thu Nov 20 22:32:58 2008
>> New Revision: 719505
>>
>> URL: http://svn.apache.org/viewvc?rev=719505&view=rev
>> Log:
>> Introduce DBM DSO linkage.
>>
>> Backports: 719504
> ...
>> Modified: apr/apr-util/branches/1.3.x/dbm/apr_dbm.c
>> URL: http://svn.apache.org/viewvc/apr/apr-util/branches/1.3.x/dbm/apr_dbm.c?rev=719505&r1=719504&r2=719505&view=diff
>> ==============================================================================
>> --- apr/apr-util/branches/1.3.x/dbm/apr_dbm.c (original)
>> +++ apr/apr-util/branches/1.3.x/dbm/apr_dbm.c Thu Nov 20 22:32:58 2008
> ...
>> +static apr_status_t dbm_open_type(apr_dbm_type_t const* * vtable,
>> + const char *type,
>> + apr_pool_t *pool)
>> +{
> ...
>> +
>> + if (!drivers)
>> + {
>> + apr_pool_t *ppool = pool;
>> + apr_pool_t *parent;
>> +
>> + /* Top level pool scope, need process-scope lifetime */
>> + for (parent = pool; parent; parent = apr_pool_parent_get(ppool))
>> + ppool = parent;
>> +
>> + /* deprecate in 2.0 - permit implicit initialization */
>> + apu_dso_init(ppool);
>> +
>> + drivers = apr_hash_make(ppool);
>> + apr_hash_set(drivers, "sdbm", APR_HASH_KEY_STRING, &apr_dbm_type_sdbm);
>
> This surely isn't thread-safe? Two threads could enter apr_dbm_open*()
> with drivers == NULL and then call apu_dso_init() on the same global
> pool, which would break.
That is correct.
Let's explore a little deeper, though?
http://svn.apache.org/repos/asf/apr/apr-util/trunk/misc/apu_dso.c
dsos' is a singleton, and apu_dso_init() may be (safely) called multiple
times to ensure the dso services are available.
If nobody has performed any database, sql, ldap or other access prior to
thread creation and race-for-first, then you are right. It won't break,
but it certainly can cause a double-load artifact due to the race.
The point to the global shared pool is that the process-loaded provider
is global to all players.
The contract needs firming up, but "break" isn't quite the right word.
Redundant, certainly, and in 2.0 we have a chance to make all these
mechanics simpler.
But this is no different than the current (and former) loadable dbd
provider issue.