Re: [xml] xmlCleanupParser() question / patch

[James Hart <beeblebrox42 gmail com>]

Do you suspect that there are more broken implementation vs.
implementations correctly using xmlCleanupParser?  The proposed change
would break correctly implemented systems, but still compile and be
hard to detect.  Shouldn't correctly implemented systems be favored
over the broken ones?

For example, in our system we have specifically used a RAII design
pattern so multiple libraries can access libxml and the initialization
and cleanup of the library is handled by a reference counting systems
because we have no control of the order these libraries will be loaded
in our system.  The goal we have is to completely unload and cleanup
the library when there are no libraries requiring it's use anymore and
to reload it if a library requires it again.  If the API changes
functionality, yet is link compatible it is very possible the change
you proposed will go unnoticed for some time.

If you rename the function I would prefer you to remove the old
signature completely so current working implementations will know they
need to change.  Current broken implementations would need to change
too...but now maybe they would have the 'ahh haa' moment and know why
they should not have been calling that function to begin with.  Of
course, I would prefer not to have to change production code simply so
broken implementations are catered to, but I understand the importance
of clearly named functions.

Thanks for an excellent library,
  James


On Tue, Jan 19, 2010 at 7:25 AM, Daniel Veillard <veillard redhat com> 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.
>
>  Opinions ?
>
> Daniel
>
>
> --
> Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
> daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
> http://veillard.com/ | virtualization library  http://libvirt.org/
> _______________________________________________
> xml mailing list, project page  http://xmlsoft.org/
> xml gnome org
> http://mail.gnome.org/mailman/listinfo/xml



-- 
If there is anything bigger than my ego around, I want it caught and shot now!