"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!
> 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...
> +
> /* 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.
> + 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 :)
Read the comment that follows the above code, it explains why using
NULL required you to change the ra_loader code.
> + 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?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Aug 2 02:59:13 2006