Re: [xml] xmlCleanupParser() question / patch

[Rob Richards <rrichards cdatazone org>]

Daniel Veillard wrote:
> On Thu, Jan 14, 2010 at 02:59:50AM +0100, Aron Szabo wrote:
>   
> > Hi!
> >
> > If I use the library in a threaded program and call
> > xmlCleanupParser() I can't reinitialize it.
> >
> > So here's what happens:
> >
> > 1. xmlCleanupThreads() is called from xmlCleanupParser()
> > 2. the global key is deleted by xmlCleanupParser() which was created
> > by xmlOnceInit()
> >
> > So the problem is that if I want to clear the data allocated then I
> > can't use the library because it will leak memory (xmlOnceInit will
> > not be called). The simplest way to solve this issue is to reset
> > once_control.
> >
> > The other way is to remove xmlCleanupThreads() from the cleanup
> > process and let the user/programmer do the dirty work...
> >
> > Aron Szabo
> > Pointless Software
> >     
>
>   
> > diff --git a/threads.c b/threads.c
> > index 98fd2c2..7f421dd 100644
> > --- a/threads.c
> > +++ b/threads.c
> > @@ -911,8 +911,10 @@ xmlCleanupThreads(void)
> >      xmlGenericError(xmlGenericErrorContext, "xmlCleanupThreads()\n");
> >  #endif
> >  #ifdef HAVE_PTHREAD_H
> > -    if ((libxml_is_threaded)  && (pthread_key_delete != NULL))
> > +    if ((libxml_is_threaded)  && (pthread_key_delete != NULL)) {
> >          pthread_key_delete(globalkey);
> > +       once_control = PTHREAD_ONCE_INIT;
> > +    }
> >  #elif defined(HAVE_WIN32_THREADS) && !defined(HAVE_COMPILER_TLS) && (!defined(LIBXML_STATIC) || 
> > defined(LIBXML_STATIC_FOR_DLL))
> >      if (globalkey != TLS_OUT_OF_INDEXES) {
> >          xmlGlobalStateCleanupHelperParams *p;
> >     
>
>
>   That makes sense. there is a big problem anyway with
> xmlCleanupParser() most people seems to think it need to be used after
> parsing while it's a library global operation and really should not
> be called after parsing but only when the library is not in use anymore.
>
> So overall I'm tempted to change xmlCleanupParser() to become an empty
> routine, and rename the real function as xmlCleanupLibrary().
> That way the various shared libraries deployed with that bug won't need
> to be changed specifically, avoiding crashes when multiple libraries
> using libxml2 are used. Then your patch should be applied to the new
> xmlCleanupLibrary() function.
>
>   
Personally I don't think the purpose of that function should be changed 
just for the fact that it has been functional *and documented* to work 
like it does now - so misuse is the developers fault. Existing apps that 
have properly used the function should not be penalized as I do know 
many environments where memory leaks are just not acceptable regardless 
of how small they are; especially when it is an intended breakage.

What I think would be better would be would be to mark xmlInitParser and 
xmlCleanupParser as depreciated but leave their current behavior. Add 
new functionals (xmlInitLibrary, xml ClenaupLibrary), as well as some 
new functions to handle local parser cleanup - like xmlParserInit and 
xmlParserCleanup (or something along those lines).

If the direction really ends up being xmlCleanupParser being made into a 
noop, I would really hate to see that done in an incremental release 
version.

Rob