On 8/1/06, Philip Martin <philip@codematters.co.uk> wrote:
> "Garrett Rooney" <rooneg@electricjellyfish.net> writes:
>
> > Ok, so here's my first cut at a patch. The fs loader stuff seems
> > pretty reasonable, but for some totally unclear reason it is not
> > enough to make the problem go away, I also have to make the ra loader
> > code allocate dsos in a global pool, if I don't then I continue to get
> > segfaults during global pool destruction. Why?
>
> Using ra_local? Loading ra_local as a DSO pulls in libsvn_fs using
> the same pool used to to load ra_local. If libsvn_fs then registers a
> cleanup on a global pool it will get called after the ra_local pool
> has unloaded libsvn_fs. Boom!
Yeah actually, it is ra_local. That would explain it...
> > I have no clue. Did
> > this stuff ever work?
> >
> > Note that this is totally not committable at this point, but I'd LOVE
> > if someone else could look at it and tell me what they think...
> >
> > -garrett
> >
> > [[[
> > Make a valiant attempt at curing the insanity of the dso loading code.
> >
> > * subversion/libsvn_fs/fs-loader.c
> > (dso_cache): New global hash of dso objects.
> > (load_module): Add locking, use the global common pool to hold the
> > dso, and use the new cache.
> >
> > * subversion/libsvn_ra/ra_loader.c
> > (load_ra_module): Use a global pool to hold the dso for the ra layer
> > since for some reason we crash during apr_terminate otherwise.
> > ]]]
> >
> > Index: subversion/libsvn_ra/ra_loader.c
> > ===================================================================
> > --- subversion/libsvn_ra/ra_loader.c (revision 20926)
> > +++ subversion/libsvn_ra/ra_loader.c (working copy)
> > @@ -143,6 +143,8 @@
> > funcname = apr_psprintf(pool, "svn_ra_%s__init", ra_name);
> > compat_funcname = apr_psprintf(pool, "svn_ra_%s_init", ra_name);
> >
> > + pool = svn_pool_create(NULL);
>
> That looks like a leak, a long running application that calls
> svn_ra_open repeatedly will allocate global pools until memory is
> exhausted. I guess that's TSVN...
This is the part that wasn't nearly ready for prime time ;-)
I just put it in so it would stop crashing while I got the other part to work.
> > +
> > /* find/load the specified library */
> > status = apr_dso_load(&dso, libname, pool);
> > if (status)
> > Index: subversion/libsvn_fs/fs-loader.c
> > ===================================================================
> > --- subversion/libsvn_fs/fs-loader.c (revision 20926)
> > +++ subversion/libsvn_fs/fs-loader.c (working copy)
> > @@ -49,6 +49,7 @@
> > #if APR_HAS_THREADS
> > static apr_thread_mutex_t *common_pool_lock;
> > #endif
> > +static apr_hash_t *dso_cache;
> >
> >
> > /* --- Utility functions for the loader --- */
> > @@ -92,13 +93,41 @@
> > name, SVN_VER_MAJOR);
> > funcname = apr_psprintf(pool, "svn_fs_%s__init", name);
> >
> > - /* Find/load the specified library. If we get an error, assume
> > - the library doesn't exist. The library will be unloaded when
> > - pool is destroyed. */
> > - status = apr_dso_load(&dso, libname, pool);
> > +#if APR_HAS_THREADS
> > + status = apr_thread_mutex_lock(common_pool_lock);
> > if (status)
> > - return SVN_NO_ERROR;
> > + return svn_error_wrap_apr(status, _("Can't grab FS mutex"));
> > +#endif
> >
> > + dso = apr_hash_get(dso_cache, libname, APR_HASH_KEY_STRING);
> > + if (! dso)
> > + {
> > + /* Find/load the specified library. If we get an error, assume
> > + the library doesn't exist. The library will be unloaded when
> > + global pool is destroyed. */
> > + status = apr_dso_load(&dso, libname, common_pool);
>
> If we keep allocating from common_pool, and that call does allocate a
> small amount, then theoretically a long running application will run
> out of memory. I think a threaded svnserve is such an application.
>
> I suppose a solution is to use a global pool per DSO and arrange for
> the BDB env stuff to use the same global pool as the DSO, and then to
> refcount the DSO loads so that the global pools can be destroyed.
Well, yes, but we only ever open a fairly small number of dsos, they
get cached in the global hash, so at most we're talking what, three of
them? Seems like overkill to care about subpools.
> > + if (status)
> > + {
> > +#if APR_HAS_THREADS
> > + status = apr_thread_mutex_unlock(common_pool_lock);
> > + if (status)
> > + return svn_error_wrap_apr(status, _("Can't ungrab FS mutex"));
> > +#endif
> > + return SVN_NO_ERROR;
> > + }
> > +
> > + apr_hash_set(dso_cache,
> > + apr_pstrdup(common_pool, libname),
> > + APR_HASH_KEY_STRING,
> > + dso);
> > + }
> > +
> > +#if APR_HAS_THREADS
> > + status = apr_thread_mutex_unlock(common_pool_lock);
> > + if (status)
> > + return svn_error_wrap_apr(status, _("Can't ungrab FS mutex"));
> > +#endif
> > +
> > /* find the initialization routine */
> > status = apr_dso_sym(&symbol, dso, funcname);
> > if (status)
> > @@ -245,7 +274,8 @@
> > if (common_pool)
> > return SVN_NO_ERROR;
> >
> > - common_pool = svn_pool_create(pool);
> > + common_pool = svn_pool_create(NULL);
>
> The pool argument to svn_fs_initialize is no longer used at all,
> that's a bit odd considering the efforts we make to explain the pool
> in the documentation :)
Heh. Well, it's used to allocate a temporary string, but yeah ;-)
> Read the comment that follows the above code, it explains why using
> NULL required you to change the ra_loader code.
Good to know. I'll look into this more tomorrow if nobody beats me to it.
> > + dso_cache = apr_hash_make(common_pool);
> > #if APR_HAS_THREADS
> > status = apr_thread_mutex_create(&common_pool_lock,
> > APR_THREAD_MUTEX_DEFAULT, common_pool);
>
> Rather than maintain the FS and RA loaders separately I wonder if they
> could use a common implementation?
Yeah, I suspect they probably should. Now that I know why the ra side
matters I can dive into abstracting this out with a clear conscience.
Thanks for the sharp eyes Philip!
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Aug 2 03:08:47 2006