Garrett Rooney wrote:
[...]
> We actually do need a destructor callback for
> apr_threadkey_private_create. If we don't pass one then any thread
> that messes with a bdb env but doesn't correctly clean it up will leak
> memory. This won't be called in the normal case, since the variable
> will have been set to NULL in the __close function, but for the
> abnormal case it is still required.
>
> Side note here... It looks like this callback will only ever be
> called on Unix, Win32 doesn't even do anything with it. Still, it's
> not like we're relying on it for anything other than avoiding leaks
> with poorly behaved users of the API, so this bug in APR doesn't
> necessarily hurt us too badly.
Ah, you noticed. :)
> There may also be a potential for races during shutdown where
> cleanup_env happens before svn_fs_bdb__close finishes up. I have no
> idea how this could happen, but I'm willing to accept on faith that it
> can. It sounds like Branko also thinks this is possible, but can't
> prove that fact. To avoid this, I think the best bet is to go with
> Branko's idea of just not doing the cleanup in the case where
> bdb->pool is NULL.
Actually I hope it's not possible ...
> Note that if that race can occur, then we're kind of stuck leaking
> something. The destruction of the thread keys in cleanup_env means
> that any access to them at all is pretty much out of the question,
> even if they were allocated in this thread. Anything that was
> allocated in other threads is already lost anyway, since we can't get
> at those thread local variables anymore since the threads are gone.
> On unix at least the cleanup function has a chance to help us with
> those...
... but as far as I can determine, it can only potentially happen during
apr_terminate, and a bit of a leak then isn't such a horrible thing.
> So, we check for the NULL pool in __close, and that keeps us from
> theoretically messing with a threadkey that's no longer valid.
>
> Note that there are almost certainly still error conditions that we
> can't do much about. Shutting down while other threads are still
> running, for example, seems fraught with danger, and I'm not sure
> there's really much we can do about it.
Shutting down while threads are still active is _always_ a problem, not
just in this case.
(On that note, I think I just solved the halting problem: if it doesn't
halt by itself, pull the plug ... ah, never mind ...)
> Anyway, here's the patch + log, let me know if it seems sane...
>
> -garrett
>
> [[[
> Fix some potential issues when cleaning up BDB environments.
>
> * subversion/libsvn_fs_base/bdb/env.c
> (cleanup_error_info): Cleanup callback for use with thread local
> variables.
> (create_env): Pass cleanup_error_info into apr_threadkey_private_create.
> (svn_fs_bdb__close): Don't try to clean anything up if bdb->pool is
> NULL,
> it implies that we've hit some strange pool cleanup ordering issue.
> ]]]
Haven't tested this, but it looks fine. Thanks for picking up on this,
Garrett.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 4 06:18:30 2006