svn_utf_initialize and pool cleanup

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

The signature of our UTF-8 initialization hack is:

  void svn_utf_initialize (void)

Peter initially had it take a pool argument, but then changed it to
allocate a subpool of the global pool, and made it clean up the xlate
handle at apr_terminate() time.

My concern here is that if someone dynamically loads the Subversion
libraries and then unloads them, this pool will be leaked each time
(unless the application initializes and terminates APR on each load,
but that's not a fair assumption to make, since the application might
be using APR for other reasons).

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.

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

Re: svn_utf_initialize and pool cleanup

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

On Mon, 6 Sep 2004, Greg Hudson wrote:

> The signature of our UTF-8 initialization hack is:
>
>   void svn_utf_initialize (void)
>
> Peter initially had it take a pool argument, but then changed it to
> allocate a subpool of the global pool, and made it clean up the xlate
> handle at apr_terminate() time.
>
> My concern here is that if someone dynamically loads the Subversion
> libraries and then unloads them, this pool will be leaked each time
> (unless the application initializes and terminates APR on each load,
> but that's not a fair assumption to make, since the application might
> be using APR for other reasons).
>
> 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.
>
OK. I didn't think of that scenario at all. I think that change is
straightforward. I wanted to simplify things by handling the cleanup
internally, and since we need to create a subpool for our private use
(protected by the mutex), I just removed the the pool argument.

> 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.
>
You're right. It's a regression if you don't use the init function. I'll
readd that caching.

I'll look at these ASAP, hopefully meaning this week.

Still I don't like to rush in this change this late in the release
process. IMO it has to wait until 1.1.1 since it needs more testing on
differet platforms in applications that actually are multi-threaded.

Thanks for the comments, Greg
//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 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

[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