On Tue, 17 Aug 2004, Philip Martin wrote:
> "Peter N. Lundblad" <peter@famlundblad.se> writes:
>
> > +void
> > +svn_utf_initialize(apr_pool_t *pool)
> > +{
> > +#if APR_HAS_THREADS
> > + apr_threadkey_t *key;
> > + if (utf_initialized++ == 0)
> > + {
> > + if (apr_threadkey_private_create(&key, xlate_cleanup, pool)
> > + != APR_SUCCESS)
> > + return;
>
> I don't see a call to apr_threadkey_private_destroy. Is this a
> resource leak? It's probably trivial.
>
You're right. It isn't destroyed by the pool (strangely enough), so
apr_threadkey_private_delete needs to be registered in the pool
explicitly.
> > + xlate_handle_key = key;
> > + }
> > +#endif
> > +}
> > +
> > /* Return an apr_xlate handle for converting from FROMPAGE to
> > TOPAGE. Create one if it doesn't exist in USERDATA_KEY. If
> > unable to find a handle, or unable to create one because
> > @@ -50,11 +83,47 @@
> > {
> > void *old_handle = NULL;
> > apr_status_t apr_err;
> > + apr_pool_t *handle_pool = NULL;
> > +#if APR_HAS_THREADS
> > + apr_hash_t *handle_hash = NULL;
> > +#endif
> >
> > /* If we already have a handle, just return it. */
> > if (userdata_key)
> > {
> > - apr_pool_userdata_get (&old_handle, userdata_key, pool);
> > +#if APR_HAS_THREADS
> > + /* First try our thread private handle. */
> > + if (xlate_handle_key)
> > + {
> > + void *val;
> > +
> > + if (apr_threadkey_private_get (&val, xlate_handle_key) == APR_SUCCESS)
> > + {
> > + handle_hash = val;
> > + }
> > +
> > + if (handle_hash)
> > + {
> > + old_handle = apr_hash_get(handle_hash, userdata_key,
> > + APR_HASH_KEY_STRING);
> > + handle_pool = apr_hash_pool_get (handle_hash);
> > + }
> > + else
> > + {
> > + handle_pool = svn_pool_create (NULL);
>
> This creates a pool in every thread, but I don't see the pool getting
> cleared or destroyed. An application that creates threads continually
> (e.g. threaded svnserve) will cause the number of pools to grow
> without bound.
>
xlate_cleanup will take care of that. It is called when the thread is
destoyed.
Problems is, when I check the implementation of
apr_threadkey_private_create, the destructor is only used on POSIX and
netware systems, not on Windows/OS2. So on these platforms, it is a leak
after all. Maybe there is no way to register cleanups on these
platforms,. This should have been documented in the APR headers!!!
> > + handle_hash = apr_hash_make (pool);
> > + if (apr_threadkey_private_set (handle_hash, xlate_handle_key)
>
> If pool is cleared handle_hash will become invalid, but will still be
> referenced by the thread private data. The next time the thread calls
> get_xlate_handle it will crash, or do something worse.
>
When the pool is cleared, the thread is dying, so the TSD for that thread
isn't used again. This would be true where the pool is destroyed at all...
Thanks for the review. Currently, I don't know how to get further.
Creating a set of xlate handles per thread is a leak, since there is no
way to clean it up. Caching the handles in parent pools doesn't work,
since that's not thread-safe. Well... Anyone has an idea??? Maybe we could
check if it is possible to decrease the number of translations by some
factor? I'll give it a try.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 17 22:18:51 2004