BBD Pool Cleanup Ordering Bugs?

[Garrett Rooney <ro...@electricjellyfish.net>]

So, in an effort to get us past the last of these BDB issues and on to
a final 1.4.0 release I've been staring at the BDB environment code
for the past few hours, trying to figure out what Brane was talking
about with regard to the last remaining pool cleanup ordering issue.
Judging from the comments in his last commit, it's something related
to this section of code in svn_fs_bdb__close:

  if (0 == --bdb_baton->error_info->refcount)
    {
      /* ###
         Oh bother, this is another potential pool-cleanup-ordering bug.
         Should we just skip this line if bdb->pool==0? See cleanup_env. */
      svn_error_clear(bdb_baton->error_info->pending_errors);
#if APR_HAS_THREADS
      free(bdb_baton->error_info);
      apr_threadkey_private_set(NULL, bdb->error_info);
#endif
    }

So clearly he's worried about clearing these errors in some situation
that occurs during global pool cleanup.  If you look at cleanup_env,
this is what you find:

static apr_status_t
cleanup_env(void *data)
{
  bdb_env_t *bdb = data;
  bdb->pool = NULL;
  bdb->dbconfig_file = NULL;   /* will be closed during pool destruction */
#if APR_HAS_THREADS
  apr_threadkey_private_delete(bdb->error_info);
#endif /* APR_HAS_THREADS */

  /* If there are no references to this descriptor, free its memory here,
     so that we don't leak it if create_env returns an error.
     See bdb_close, which takes care of freeing this memory if the
     environment is still open when the cache is destroyed. */
  if (!bdb->refcount)
    free(data);

  return APR_SUCCESS;
}

So Brane's comment implies that maybe if bdb->pool is already NULL we
shouldn't bother dealing with bdb->error_info, presumably since it's
already been destroyed via apr_threadkey_private_delete.  I'm not
entirely sure this is correct though.  We didn't register any
destructor when we created the thread-local variable, so it won't have
actually destroyed the errors.  Do we have to put code clearing those
errors into the cleanup_env function, so that if it's called before
the __close code it can clean up the errors, then delete the thread
local var?  It'd also have to do the free(), I imagine...  The __close
code would then have to check for bdb->pool before it does the clear
and free, right?

Am I on the right track here?  Anyone?  This stuff is kind of voodoo,
and I'm not all that clear that I'm even close to understanding the
actual problem...

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: BBD Pool Cleanup Ordering Bugs?

[Garrett Rooney <ro...@electricjellyfish.net>]

On 7/4/06, Branko Čibej <br...@xbc.nu> wrote:

> Haven't tested this, but it looks fine. Thanks for picking up on this,
> Garrett.

Ok, committed to trunk.  Will start testing the merge of this and your
other change (using malloc to manage that memory) into 1.4 and if all
goes well I'll propose it for backport.

-garrett

Re: BBD Pool Cleanup Ordering Bugs?

[Branko Čibej <br...@xbc.nu>]

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

Re: BBD Pool Cleanup Ordering Bugs?

[Garrett Rooney <ro...@electricjellyfish.net>]

On 6/30/06, Branko Čibej <br...@xbc.nu> wrote:

> Sorry I sort of ignored your last mail, I've been quite busy lately. But
> it turns out that you guessed what worries me.

Ok, good to know I'm at least on the right track ;-)

> > Another question that jumps to mind, how does this stuff even work
> > with thread local variables like this?  Does it imply that __close
> > needs to be called from the same thread that was using the environment
> > (and thus that generated the errors) so that it can free the errors?
> > If that's the case, can cleanup_env even potentially destroy those
> > errors, how would it get at them?  I feel like I'm missing something
> > here...
> The idea is that if you create an svn_fs_t in one thread, you must
> always use it in that thread. I don't think we've spelled that out
> anywhere, but then, we've not really defined our thread-safety
> guarantees in general. Certainly a threaded svnserve uses svn_fs_t's in
> the way that code expects them to be used.

Yeah...  This kind of worries me, but it's not like we can really do
much better.

> I'm worried that it's possible to order svn_fs_t creation in such a way
> that the gathered svn_error_t's are destroyed before the last svn_fs_t
> that might want to use them. At this point, I don't even know if this
> scenario is possible, much less how to recover from it.

I think we're basically stuck in some of the potential situations.

Here's my current thinking, and my current patch.  Let me know if this
makes any sense at all, because currently I'm not quite sure ;-)

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.

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.

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...

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.

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.
]]]

Re: BBD Pool Cleanup Ordering Bugs?

[Branko Čibej <br...@xbc.nu>]

Garrett Rooney wrote:
> On 6/30/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
>
>> So Brane's comment implies that maybe if bdb->pool is already NULL we
>> shouldn't bother dealing with bdb->error_info, presumably since it's
>> already been destroyed via apr_threadkey_private_delete.  I'm not
>> entirely sure this is correct though.  We didn't register any
>> destructor when we created the thread-local variable, so it won't have
>> actually destroyed the errors.  Do we have to put code clearing those
>> errors into the cleanup_env function, so that if it's called before
>> the __close code it can clean up the errors, then delete the thread
>> local var?  It'd also have to do the free(), I imagine...  The __close
>> code would then have to check for bdb->pool before it does the clear
>> and free, right?
>>
>> Am I on the right track here?  Anyone?  This stuff is kind of voodoo,
>> and I'm not all that clear that I'm even close to understanding the
>> actual problem...
Sorry I sort of ignored your last mail, I've been quite busy lately. But
it turns out that you guessed what worries me.

> Another question that jumps to mind, how does this stuff even work
> with thread local variables like this?  Does it imply that __close
> needs to be called from the same thread that was using the environment
> (and thus that generated the errors) so that it can free the errors?
> If that's the case, can cleanup_env even potentially destroy those
> errors, how would it get at them?  I feel like I'm missing something
> here...
The idea is that if you create an svn_fs_t in one thread, you must
always use it in that thread. I don't think we've spelled that out
anywhere, but then, we've not really defined our thread-safety
guarantees in general. Certainly a threaded svnserve uses svn_fs_t's in
the way that code expects them to be used.

I'm worried that it's possible to order svn_fs_t creation in such a way
that the gathered svn_error_t's are destroyed before the last svn_fs_t
that might want to use them. At this point, I don't even know if this
scenario is possible, much less how to recover from it.

> Have I mentioned that I'm not overly fond of thread local variables
> like this?  Man do they make things confusing...
You don't say.

-- Brane

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: BBD Pool Cleanup Ordering Bugs?

[Garrett Rooney <ro...@electricjellyfish.net>]

On 6/30/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:

> So Brane's comment implies that maybe if bdb->pool is already NULL we
> shouldn't bother dealing with bdb->error_info, presumably since it's
> already been destroyed via apr_threadkey_private_delete.  I'm not
> entirely sure this is correct though.  We didn't register any
> destructor when we created the thread-local variable, so it won't have
> actually destroyed the errors.  Do we have to put code clearing those
> errors into the cleanup_env function, so that if it's called before
> the __close code it can clean up the errors, then delete the thread
> local var?  It'd also have to do the free(), I imagine...  The __close
> code would then have to check for bdb->pool before it does the clear
> and free, right?
>
> Am I on the right track here?  Anyone?  This stuff is kind of voodoo,
> and I'm not all that clear that I'm even close to understanding the
> actual problem...

Another question that jumps to mind, how does this stuff even work
with thread local variables like this?  Does it imply that __close
needs to be called from the same thread that was using the environment
(and thus that generated the errors) so that it can free the errors?
If that's the case, can cleanup_env even potentially destroy those
errors, how would it get at them?  I feel like I'm missing something
here...

Have I mentioned that I'm not overly fond of thread local variables
like this?  Man do they make things confusing...

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org