[PATCH] Re: svn_utf_initialize and pool cleanup

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Mon, 6 Sep 2004, Greg Hudson wrote:

> So I do think we want to be passing a pool argument to
> svn_utf_initialize(), with the proviso that no Subversion functions
> may be in progress when this pool is cleaned up.  The pool cleanup
> handler can reset the static data so that we go back to per-pool xlate
> handles.
>
> Speaking of which, it looks like Peter's code doesn't use per-pool
> xlate handles when svn_utf_initialize() is called, which means his
> change makes translation much *slower* if the initializer isn't
> called.  That doesn't seem acceptable.
>
The attached tries to fix the above problems. It is getting complex - I
won't commit before I get a review or two:-)

BTW, I ran into an interesting bug that we must have had in the old code
without it showing up. When we create and cache an xlate handle from
inside a pool cleanup handler, the handle will be closed immediately after
the return of that cleanup and the cache will be invalid. I think we are
lucky this hasn't manifested itself, but now when we have a global cache
that is destroyed in a pool cleanup, it showed up...

Also, I had to move the call of svn_Utf_initialize out of
svn_cmdline_initialize, since we don't have a pool there. I haven't added
it to svnserve, since it creates detached threads and I don't know how to
guarantee that they are done before the cleanup of the pool, which is a
requirement.

Regards,
//Peter

Re: [PATCH] Re: svn_utf_initialize and pool cleanup

[Greg Hudson <gh...@MIT.EDU>]

On Thu, 2004-09-09 at 16:09, Peter N. Lundblad wrote:
> This is messy. Hope I expressed my self somewhat clearly...

I understand now.

On a related note, I've received agreement from some people on IRC that
we should add an empty stub svn_utf_initialize() to the API for
1.1.0-rc3, so that we can put the fix into 1.1.1 without violating our
API constraints.  We can test your code on the trunk and in tsvn
(assuming Steve is going to apply it locally to tsvn releases prior to
1.1.1) until then.  I'm going to take care of making the stub-API
proposal in the STATUS file now.


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

Re: [PATCH] Re: svn_utf_initialize and pool cleanup

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Thu, 9 Sep 2004, Greg Hudson wrote:

> On Thu, 2004-09-09 at 14:53, Peter N. Lundblad wrote:
> It took me a while to figure out why we need a private subpool (it's
> because we need to make allocations in that subpool in order to create
> new handles, and allocations within a pool aren't thread-safe).  You
> might make the comment a little more verbose.
>
Always a good reason to extend a comment.

> +  /* If we are called from inside a pool cleanup handler, the just created
> +     xlate handle will be closed when that handler returns by a newly
> +     registered cleanup handler, however, the handle is still cached by us.
>
> I don't understand this.  We allocate the xlate handle in the pool
> created by svn_utf_initialize(), right?  And presumably *that* pool has
> no cleanup handlers which invoke translation functions, since we own the
> pool.  So why would the cleanup handler fire off right away?
>
This happens when the global cache isn't used anymore and therefore the
per-pool cache is used. Then if we create a handle in a clean-up function,
right after that, the handle will be closed and the next cleanup function
that needs a handle will crash. You can try it if you want to. Just remove
this part and try svnversion in a directory with subdirectories. Also,
remember that a pool will run its own cleanups first, then the subpool
cleanups. So, if we have subpool with cleanups that do char xlations and
they are cleaned up by the global pool that was passed to
svn_utf_initialize, our cache will be destroyed before thos subpool
cleanups are called and the per-pool cache will be used, but not in many
cases, since the handles will be closed after each cleanup. I think we
have this problem in the old code too. We've just been lucky that a handle
was created earlier in this case when the per-pool cache was used all the
time.

This is messy. Hope I expressed my self somewhat clearly...

Regards,
//Peter

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

Re: [PATCH] Re: svn_utf_initialize and pool cleanup

[Greg Hudson <gh...@MIT.EDU>]

On Thu, 2004-09-09 at 14:53, Peter N. Lundblad wrote:
> > It should be fine to create a pool in svn_cmdline_initialize.  A program
> > dynamically loading and unloading the svn libraries won't use that
> > function.
> >
> Goodi point. Is guaranteed somewhere or just a reasonable assuptiion?

I think it's a reasonable assumption given what that function does.

> Did you look at the patch in itself?

Whoops, I meant to.

It took me a while to figure out why we need a private subpool (it's
because we need to make allocations in that subpool in order to create
new handles, and allocations within a pool aren't thread-safe).  You
might make the comment a little more verbose.

+  /* If we are called from inside a pool cleanup handler, the just created
+     xlate handle will be closed when that handler returns by a newly
+     registered cleanup handler, however, the handle is still cached by us.

I don't understand this.  We allocate the xlate handle in the pool
created by svn_utf_initialize(), right?  And presumably *that* pool has
no cleanup handlers which invoke translation functions, since we own the
pool.  So why would the cleanup handler fire off right away?

The patch looks fine otherwise.


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

Re: [PATCH] Re: svn_utf_initialize and pool cleanup

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Thu, 9 Sep 2004, Greg Hudson wrote:

> On Wed, 2004-09-08 at 17:37, Peter N. Lundblad wrote:
> > Also, I had to move the call of svn_Utf_initialize out of
> > svn_cmdline_initialize, since we don't have a pool there.
>
> It should be fine to create a pool in svn_cmdline_initialize.  A program
> dynamically loading and unloading the svn libraries won't use that
> function.
>
Goodi point. Is guaranteed somewhere or just a reasonable assuptiion?

> >  I haven't added
> > it to svnserve, since it creates detached threads and I don't know how to
> > guarantee that they are done before the cleanup of the pool, which is a
> > requirement.
>
> I'm not sure where your concern arises from that the pool in main()
> might be cleaned up before the detached threads terminate.  As far as I
> can tell, we never clean up that pool, because the main loop never
> terminates.
>
It will be fixed if the above is changed.

Did you look at the patch in itself?

Thanks,
//Peter

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

Re: [PATCH] Re: svn_utf_initialize and pool cleanup

[Greg Hudson <gh...@MIT.EDU>]

On Wed, 2004-09-08 at 17:37, Peter N. Lundblad wrote:
> Also, I had to move the call of svn_Utf_initialize out of
> svn_cmdline_initialize, since we don't have a pool there.

It should be fine to create a pool in svn_cmdline_initialize.  A program
dynamically loading and unloading the svn libraries won't use that
function.

>  I haven't added
> it to svnserve, since it creates detached threads and I don't know how to
> guarantee that they are done before the cleanup of the pool, which is a
> requirement.

I'm not sure where your concern arises from that the pool in main()
might be cleaned up before the detached threads terminate.  As far as I
can tell, we never clean up that pool, because the main loop never
terminates.

(If we ever want to add graceful termination to svnserve, we'll have to
join the detached threads, I think; otherwise we won't be able to call
apr_terminate().)


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