why svn object pools? (Re: svn commit: r1782614 - /subversion/trunk/subversion/libsvn_repos/authz.c)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

why svn object pools? (Re: svn commit: r1782614 - /subversion/trunk/subversion/libsvn_repos/authz.c)

Stefan Sperling-9
On Sat, Feb 11, 2017 at 04:07:07PM -0000, [hidden email] wrote:

> Author: stsp
> Date: Sat Feb 11 16:07:06 2017
> New Revision: 1782614
>
> URL: http://svn.apache.org/viewvc?rev=1782614&view=rev
> Log:
> Fix a crash during init in libsvn_repos with pool debugging enabled.
>
> * subversion/libsvn_repos/authz.c
>   (synchronized_authz_initialize): With APR_POOL_DEBUG, the function
>    apr_pool_allocator_get() will return NULL (there is no global allocator).
>    So dereferencing that pointer to find the allocator's mutex will segfault.
>    It is unclear to me if using apr_allocator_mutex_get() is a sane way to find
>    out if we're running in multithreaded mode, and I can't find a better way
>    right now. For now, just assume that we're multithreaded if APR has threads.
>    This should be safe and allows me to run the tests over DAV again.
>
> Modified:
>     subversion/trunk/subversion/libsvn_repos/authz.c
>
> Modified: subversion/trunk/subversion/libsvn_repos/authz.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/authz.c?rev=1782614&r1=1782613&r2=1782614&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_repos/authz.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/authz.c Sat Feb 11 16:07:06 2017
> @@ -135,8 +135,7 @@ static svn_error_t *
>  synchronized_authz_initialize(void *baton, apr_pool_t *pool)
>  {
>  #if APR_HAS_THREADS
> -  svn_boolean_t multi_threaded
> -    = apr_allocator_mutex_get(apr_pool_allocator_get(pool)) != NULL;
> +  svn_boolean_t multi_threaded = TRUE;
>  #else
>    svn_boolean_t multi_threaded = FALSE;
>  #endif
>

The reason the above code wants to know if we're using threads is that
there is a new private svn_object_pool API which requires this information.

Is this new API critical to the new authz implementation?
It seems like yet another layer which adds complexity and maintenance burden.
Could the new authz implementation work without this? What would the impact be?
Why aren't plain old APR pools good enough?
Reply | Threaded
Open this post in threaded view
|

Re: why svn object pools? (Re: svn commit: r1782614 - /subversion/trunk/subversion/libsvn_repos/authz.c)

Stefan Fuhrmann
On 11.02.2017 17:18, Stefan Sperling wrote:

> On Sat, Feb 11, 2017 at 04:07:07PM -0000, [hidden email] wrote:
>> Author: stsp
>> Date: Sat Feb 11 16:07:06 2017
>> New Revision: 1782614
>>
>> URL: http://svn.apache.org/viewvc?rev=1782614&view=rev
>> Log:
>> Fix a crash during init in libsvn_repos with pool debugging enabled.
>>
>> * subversion/libsvn_repos/authz.c
>>    (synchronized_authz_initialize): With APR_POOL_DEBUG, the function
>>     apr_pool_allocator_get() will return NULL (there is no global allocator).
>>     So dereferencing that pointer to find the allocator's mutex will segfault.
>>     It is unclear to me if using apr_allocator_mutex_get() is a sane way to find
>>     out if we're running in multithreaded mode, and I can't find a better way
>>     right now. For now, just assume that we're multithreaded if APR has threads.
>>     This should be safe and allows me to run the tests over DAV again.
>>
>> Modified:
>>      subversion/trunk/subversion/libsvn_repos/authz.c
>>
>> Modified: subversion/trunk/subversion/libsvn_repos/authz.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/authz.c?rev=1782614&r1=1782613&r2=1782614&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_repos/authz.c (original)
>> +++ subversion/trunk/subversion/libsvn_repos/authz.c Sat Feb 11 16:07:06 2017
>> @@ -135,8 +135,7 @@ static svn_error_t *
>>   synchronized_authz_initialize(void *baton, apr_pool_t *pool)
>>   {
>>   #if APR_HAS_THREADS
>> -  svn_boolean_t multi_threaded
>> -    = apr_allocator_mutex_get(apr_pool_allocator_get(pool)) != NULL;
>> +  svn_boolean_t multi_threaded = TRUE;
>>   #else
>>     svn_boolean_t multi_threaded = FALSE;
>>   #endif
>>
> The reason the above code wants to know if we're using threads is that
> there is a new private svn_object_pool API which requires this information.
>
> Is this new API critical to the new authz implementation?
> It seems like yet another layer which adds complexity and maintenance burden.
> Could the new authz implementation work without this? What would the impact be?
> Why aren't plain old APR pools good enough?
svn_object_pool instances are basically caches of long-lived
objects (potentially) used by multiple threads. So, they are
closer to apr_hash_t than apr_pool_t - plus reference counting
and object lifetime management.

The objects stored in them are parsed authz files. In many
setups, those will be small and caching them is not really
necessary. Some users, however, have huge authz rule sets.
A single user will open up to 3 HTTP connections to the server,
so it is easy to see how memory requirements can explode
if you don't have a pool of "singleton" authz objects.

-- Stefan^2.