Re: [xml] Global error (xmlLastError) not cleaned up if xmlCleanupParser() is called on a different thread!

[Alex Bligh <alex alex org uk>]

Daniel,

> > FWIW last time this all came up (possibly my fault) I was going to send
> > you a patch,
> >
> > What that would have done was:
> >
> > a) provide 2 new calls:
> >   xmlContextPtr xmlNewContext()
> >   xmlFreerContext(xmlContextPtr ctx)
> >
> > b) make these two increment a static variable, and if the variable was
> > zero on entry, do what xmlInitParser does; and decrement a static
> > variable, and if it reaches zero, do xmlCleanupParser. Make them return
> > and free an opaque xmlContext struct that currently holds nothing.
>
>  what happen is a sublibrary or a plugin uses libxml2 ? Then you crash
> the problem is that this assume a very controlled context and
> unfortunately that's not the case libxml2 is reused by a number of
> libraries.

This is the problem it was designed to address.

Let's say application A and libraries B and C both use libxml2.
None has any awareness the other is using it. The current issue is
that B and C cannot safely call xmlCleanupParser, and A can only
safely call it if it knows it's effectively never going to make
any further library calls (as strictly speaking any library could
use libxml2).

To make things worse, it is legitimate to call xmlInitParser
more than once.

Now what happens is A, B and C switch gradually to using
the new calls (which are relatively simple changes). If either
A, B or C have used xmlInitParser, then the 'new' call to
xmlFreeContext does not in fact do the cleanup - i.e. just
the counter of 'new' callers is decremented. We know
libraries don't (well, should not and are broken if they
do) call xmlCleanupParser, so the only thing that can legitimately
call it is the application (A). So if B and C are converted
to use the new calls, all clear up will happen properly and
predictably whether A is converted or not. Moreover, if
B and C are converted but application D (which uses B and
C) does NOT use libxml2, D will no longer leak memory.

This is a substantial improvement, as we'll no longer see
valgrind memory leaks in the case of D, and cleaning up after
oneself will be easier.

> > c) mark xmlInitParser and xmlCleanupParser as deprecated

That bit can be done at the same time.

> > This at least allows for multiple inits and frees between libraries,
> > but does not (yet) solve the problem of global variables. Eventually
> > we'd then make an xmlContext contain an xmlGlobalState and add
> > variants of each API call that allowed for an xmlContext to be
> > passed as the first argument; this could be relatively easily
> > done with some nasty perl on the source code.
>
>  This would work if everybody started writing fresh code around
> libxml2, but that does not reflect reality unfortunately.


To be clear what I am suggesting is continuing to support the
existing API (mapped through macros to a global context), so
existing users continue to work and the ABI is unchanged. As
the old API would be generated by macros, there would (in theory)
be minimum extra maintenance.

I'm suggesting the underlying calls take a pointer to a
global state object, so that new applications can be written
that don't assume global state. Most importantly, libraries
can be written that can work without assumptions as to whether
the calling application has set (e.g.) the tree indent thing.

Clearly people aren't going to change the API they use
immediately (hence the need to support the old API) and
we'd arrange that simply by passing a NULL as the first
argument (or similar), it used the global state, which
would mean converting an application would be simply a
matter of perl search and replace to start off with.

-- 
Alex Bligh