Bug #91800 for XML-LibXML: Threads still failing?

Thu Jan 02 14:09:57 2014 perrettdl [...] googlemail.com - Ticket created

Subject:	Threads still failing?
Date:	Thu, 2 Jan 2014 19:09:43 +0000
To:	bug-XML-LibXML [...] rt.cpan.org
From:	D Perrett <perrettdl [...] googlemail.com>

https://gist.github.com/pdl/8222887 breaks with error PmmFreeHashTable: not empty On Strawberry Perl, the error is different, it lists the memory addresses. I have observed this on 5.12 (Strawberry 32-bit), 5.14 (cygwin 32-bit, Ubuntu 64-bit, Fedora 32-bit). I have confirmed this occurs even when t/90threads.t is executed and passes and is not skipped. I don't have access to later releases compiled with threads but I can't see anything in https://metacpan.org/pod/release/SHLOMIF/XML-LibXML-2.0108/LibXML.pod#THREAD-SUPPORT or in perldelta indicating later perls will yield different results. Daniel

Thu Jan 02 14:16:14 2014 perrettdl [...] googlemail.com - Correspondence added

Subject:	Re: [rt.cpan.org #91800] AutoReply: Threads still failing?
Date:	Thu, 2 Jan 2014 19:16:06 +0000
To:	bug-XML-LibXML [...] rt.cpan.org
From:	D Perrett <perrettdl [...] googlemail.com>

NB: the error without :threads_shared is PmmREFCNT_dec: REFCNT decremented below 0 for b1fea0!. PmmREFCNT_dec: REFCNT decremented below 0 for b3edc0!. PmmREFCNT_dec: REFCNT decremented below 0 for b4a830!. PmmREFCNT_dec: REFCNT decremented below 0 for b47f40!. PmmREFCNT_dec: REFCNT decremented below 0 for b47a80!. *** Error in `perl': corrupted double-linked list: 0x0000000000b47f10 *** On Strawberry, this is always the error, except I don't get the last line, I get a dialog box.

Fri Jan 03 05:24:40 2014 SHLOMIF [...] cpan.org - Correspondence added

Hi, On Thu Jan 02 14:09:57 2014, perrettdl@googlemail.com wrote: Show quoted text

> https://gist.github.com/pdl/8222887 >

Please put the contents of that gist as an attachment here, so it won't get lost. I'll try to take a look at this problem when I'm in the mood. In the meanwhile - a patch with a regression testcase would be welcome. Regards, -- Shlomi Fish

Fri Jan 03 05:24:40 2014 The RT System itself - Status changed from 'new' to 'open'

Mon Jan 20 08:28:28 2014 perrettdl [...] googlemail.com - Correspondence added

Subject:	Re: [rt.cpan.org #91800] Threads still failing?
Date:	Mon, 20 Jan 2014 13:28:15 +0000
To:	bug-XML-LibXML [...] rt.cpan.org
From:	D Perrett <perrettdl [...] googlemail.com>

Hello I've taken the standalone file and turned it into a test file, and then attached it. For me it fails with: "Dubious, test returned 5 (wstat 1280, 0x500)" Daniel On 3 January 2014 10:24, Shlomi Fish via RT <bug-XML-LibXML@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=91800 > > > Hi, > > On Thu Jan 02 14:09:57 2014, perrettdl@googlemail.com wrote:

>> https://gist.github.com/pdl/8222887 >>

> > Please put the contents of that gist as an attachment here, so it won't get lost. > > I'll try to take a look at this problem when I'm in the mood. In the meanwhile - a patch with a regression testcase would be welcome. > > Regards, > > -- Shlomi Fish >

Message body is not shown because sender requested not to inline it.

Thu Feb 13 10:52:47 2014 yoreek [...] yahoo.com - Correspondence added

On Mon Jan 20 15:28:28 2014, perrettdl@googlemail.com wrote: Show quoted text

> Hello > > I've taken the standalone file and turned it into a test file, and > then attached it. For me it fails with: "Dubious, test returned 5 > (wstat 1280, 0x500)" > > Daniel > > On 3 January 2014 10:24, Shlomi Fish via RT <bug-XML- > LibXML@rt.cpan.org> wrote:

> > <URL: https://rt.cpan.org/Ticket/Display.html?id=91800 > > > > > Hi, > > > > On Thu Jan 02 14:09:57 2014, perrettdl@googlemail.com wrote:

> >> https://gist.github.com/pdl/8222887 > >>

> > > > Please put the contents of that gist as an attachment here, so it > > won't get lost. > > > > I'll try to take a look at this problem when I'm in the mood. In the > > meanwhile - a patch with a regression testcase would be welcome. > > > > Regards, > > > > -- Shlomi Fish > >

Hi I investigated this bug and I think the bug occurs during pass object to thread "$q->enqueue($doc)". In which case uses function "shared_clone" and new variable looks as this: SV = IV(0x2610f20) at 0x2610f28 REFCNT = 1 FLAGS = (PADMY,ROK) RV = 0x2235438 SV = PVMG(0x2317a20) at 0x2235438 REFCNT = 1 FLAGS = (PADMY,OBJECT,GMG,SMG,OVERLOAD,pIOK) IV = 39825760 NV = 0 PV = 0 MAGIC = 0x2629310 MG_VIRTUAL = 0x7f2f3c2ec440 MG_TYPE = PERL_MAGIC_shared_scalar(n) MG_FLAGS = 0x30 MG_PTR = 0x223f758 "" STASH = 0x2444540 "XML::LibXML::Document" Wherein REFCNT of the real object is not changed, but when this is destroyed, the counter is decreased, and this should not be. Basically we can rewrite the source code so as not to pass object by $q->enqueue(@docs), but I can offer patch that adds check if variable is shared and if so, not to decrease REFCNT. Sorry for my english. -- YOREEK

Subject:

rt91800_shared_clone.patch

diff -Nuraw XML-LibXML-2.0110-orig/LibXML.xs XML-LibXML-2.0110/LibXML.xs --- XML-LibXML-2.0110-orig/LibXML.xs 2014-01-31 14:49:14.000000000 +0700 +++ XML-LibXML-2.0110/LibXML.xs 2014-02-13 19:44:48.295305557 +0700 @@ -4100,8 +4100,33 @@ void DESTROY( node ) SV * node + PREINIT: + int count; + SV *is_shared; CODE: #ifdef XML_LIBXML_THREADS + if ( (is_shared = get_sv("XML::LibXML::__threads_shared", 0)) == NULL ) { + is_shared = &PL_sv_undef; + } + if ( SvTRUE(is_shared) ) { + dSP; + ENTER; + SAVETMPS; + PUSHMARK(SP); + XPUSHs(node); + PUTBACK; + count = call_pv("threads::shared::is_shared", G_SCALAR); + SPAGAIN; + if (count != 1) + croak("Couldn't checks if the variable is shared or not\n"); + is_shared = POPs; + PUTBACK; + FREETMPS; + LEAVE; + if (is_shared != &PL_sv_undef) { + XSRETURN_UNDEF; + } + } if( PmmUSEREGISTRY ) { SvLOCK(PROXY_NODE_REGISTRY_MUTEX); PmmRegistryREFCNT_dec(SvPROXYNODE(node)); diff -Nuraw XML-LibXML-2.0110-orig/t/90shared_clone_failed_rt_91800.t XML-LibXML-2.0110/t/90shared_clone_failed_rt_91800.t --- XML-LibXML-2.0110-orig/t/90shared_clone_failed_rt_91800.t 1970-01-01 07:00:00.000000000 +0700 +++ XML-LibXML-2.0110/t/90shared_clone_failed_rt_91800.t 2014-02-13 22:36:24.458723624 +0700 @@ -0,0 +1,46 @@ + +use strict; +use warnings; + +use Test::More; +use Config; +BEGIN +{ + my $will_run = 0; + if ( $Config{useithreads} ) + { + if ($ENV{THREAD_TEST}) + { + require threads; + require threads::shared; + $will_run = 1; + } + else + { + plan skip_all => "optional (set THREAD_TEST=1 to run these tests)"; + } + } + else + { + plan skip_all => "no ithreads in this Perl"; + } + + if ($will_run) + { + plan tests => 3; + } +} + +use XML::LibXML qw(:threads_shared); +# TEST +ok(1, 'Loaded'); + +my $p = XML::LibXML->new(); +# TEST +ok($p, 'Parser initted.'); + +{ + my $doc = $p->parse_string(qq{<root><foo id="1">bar</foo></root>}); + my $cloned = threads::shared::shared_clone($doc); + ok(1, "Shared clone"); +}

Wed Feb 19 06:50:17 2014 SHLOMIF [...] cpan.org - Correspondence added

I applied YOREEK's patch (after some modification) in the 195d5d278828 commit. YOREEK, for your information: 1. Next time, the patch should also include a description in the "Changes" file. 2. You missed a "# TEST" directive in the test file. 3. The test script should include the URL to the bug report in the comments or POD. Regards, -- Shlomi Fish

Thu Mar 06 07:51:04 2014 paul [...] city-fan.org - Correspondence added

From:

paul [...] city-fan.org

On Wed Feb 19 06:50:17 2014, SHLOMIF wrote: Show quoted text

> I applied YOREEK's patch (after some modification) in the 195d5d278828 > commit. YOREEK, for your information: > > 1. Next time, the patch should also include a description in the > "Changes" file. > > 2. You missed a "# TEST" directive in the test file. > > 3. The test script should include the URL to the bug report in the > comments or POD. > > Regards, > > -- Shlomi Fish

Also please note that threads::shared::shared_clone was introduced in threads::shared 1.21, so I needed to tweak the test a little to skip on older versions: --- t/90shared_clone_failed_rt_91800.t +++ t/90shared_clone_failed_rt_91800.t @@ -13,7 +13,14 @@ { require threads; require threads::shared; - $will_run = 1; + if ( $threads::shared::VERSION < 1.21 ) + { + plan skip_all => "threads::shared 1.21 required for shared_clone"; + } + else + { + $will_run = 1; + } } else { Also, is_shared was introduced in threads::shared 0.96, and I get warnings from the threads test on older versions: t/90threads............................ (in cleanup) Undefined subroutine &threads::shared::is_shared called at /builddir/build/BUILD/XML-LibXML-2.0111/blib/lib/XML/LibXML.pm line 1571. (in cleanup) Undefined subroutine &threads::shared::is_shared called at /builddir/build/BUILD/XML-LibXML-2.0111/blib/lib/XML/LibXML.pm line 1571. (in cleanup) Undefined subroutine &threads::shared::is_shared called at /builddir/build/BUILD/XML-LibXML-2.0111/blib/lib/XML/LibXML.pm line 1571. (in cleanup) Undefined subroutine &threads::shared::is_shared called at /builddir/build/BUILD/XML-LibXML-2.0111/blib/lib/XML/LibXML.pm line 1571. (in cleanup) Undefined subroutine &threads::shared::is_shared called at /builddir/build/BUILD/XML-LibXML-2.0111/blib/lib/XML/LibXML.pm line 1571. (repeat lots) (in cleanup) Undefined subroutine &threads::shared::is_shared called during global destruction. PmmFreeHashTable: not empty (in cleanup) Undefined subroutine &threads::shared::is_shared called during global destruction. PmmFreeHashTable: not empty (in cleanup) Undefined subroutine &threads::shared::is_shared called during global destruction. PmmFreeHashTable: not empty (in cleanup) Undefined subroutine &threads::shared::is_shared called during global destruction. PmmFreeHashTable: not empty (repeat lots) Perhaps _id could be used instead?

Fri Mar 07 12:16:07 2014 yoreek [...] yahoo.com - Correspondence added

On Thu Mar 06 14:51:04 2014, paul@city-fan.org wrote: Show quoted text

> On Wed Feb 19 06:50:17 2014, SHLOMIF wrote:

> > I applied YOREEK's patch (after some modification) in the > > 195d5d278828 > > commit. YOREEK, for your information: > > > > 1. Next time, the patch should also include a description in the > > "Changes" file. > > > > 2. You missed a "# TEST" directive in the test file. > > > > 3. The test script should include the URL to the bug report in the > > comments or POD. > > > > Regards, > > > > -- Shlomi Fish

> > Also please note that threads::shared::shared_clone was introduced in > threads::shared 1.21, so I needed to tweak the test a little to skip > on older versions: > > --- t/90shared_clone_failed_rt_91800.t > +++ t/90shared_clone_failed_rt_91800.t > @@ -13,7 +13,14 @@ > { > require threads; > require threads::shared; > - $will_run = 1; > + if ( $threads::shared::VERSION < 1.21 ) > + { > + plan skip_all => "threads::shared 1.21 required for > shared_clone"; > + } > + else > + { > + $will_run = 1; > + } > } > else > { > > Also, is_shared was introduced in threads::shared 0.96, and I get > warnings from the threads test on older versions: > > t/90threads............................ (in cleanup) Undefined > subroutine &threads::shared::is_shared called at > /builddir/build/BUILD/XML-LibXML-2.0111/blib/lib/XML/LibXML.pm line > 1571. > (in cleanup) Undefined subroutine &threads::shared::is_shared > called at /builddir/build/BUILD/XML-LibXML- > 2.0111/blib/lib/XML/LibXML.pm line 1571. > (in cleanup) Undefined subroutine &threads::shared::is_shared > called at /builddir/build/BUILD/XML-LibXML- > 2.0111/blib/lib/XML/LibXML.pm line 1571. > (in cleanup) Undefined subroutine &threads::shared::is_shared > called at /builddir/build/BUILD/XML-LibXML- > 2.0111/blib/lib/XML/LibXML.pm line 1571. > (in cleanup) Undefined subroutine &threads::shared::is_shared > called at /builddir/build/BUILD/XML-LibXML- > 2.0111/blib/lib/XML/LibXML.pm line 1571. > (repeat lots) > (in cleanup) Undefined subroutine &threads::shared::is_shared > called during global destruction. > PmmFreeHashTable: not empty > (in cleanup) Undefined subroutine &threads::shared::is_shared > called during global destruction. > PmmFreeHashTable: not empty > (in cleanup) Undefined subroutine &threads::shared::is_shared > called during global destruction. > PmmFreeHashTable: not empty > (in cleanup) Undefined subroutine &threads::shared::is_shared > called during global destruction. > PmmFreeHashTable: not empty > (repeat lots) > > Perhaps _id could be used instead?

I agree with Paul that we can use "_id" instead of "is_shared": --- XML-LibXML-2.0111-orig/LibXML.xs 2014-03-05 17:13:09.000000000 +0200 +++ XML-LibXML-2.0111/LibXML.xs 2014-03-07 20:27:47.000000000 +0200 @@ -4127,7 +4127,7 @@ PUSHMARK(SP); XPUSHs(node); PUTBACK; - count = call_pv("threads::shared::is_shared", G_SCALAR); + count = call_pv("threads::shared::_id", G_SCALAR); SPAGAIN; if (count != 1) croak("Couldn't checks if the variable is shared or not\n"); -- YOREEK

Fri Mar 07 14:29:18 2014 NWELLNHOF [...] cpan.org - Correspondence added

RT-Send-CC:

yoreek [...] yahoo.com

On Thu Feb 13 10:52:47 2014, yoreek wrote: Show quoted text

> Basically we can rewrite the source code so as not to pass object by > $q->enqueue(@docs), but I can offer patch that adds check if variable > is shared and if so, not to decrease REFCNT.

Unfortunately, this can't be made to work safely. Consider the following sequence: my $doc = $parser->parse(...); my $clone = shared_clone($doc); $doc = undef; print $clone->documentElement; When $doc is set to undef, it will be freed and subsequent usage via the cloned SVs will fail (segfault or whatever). The way I see it, it's simply impossible to make shared_clone work with XML::LibXML objects. So I'd vote to revert your patch. But it should work to simply pass a reference to an XML::LibXML object to $q->enqueue. $q->enqueue(map { \$_ } @docs);

Sat Mar 08 04:25:28 2014 yoreek [...] yahoo.com - Correspondence added

On Fri Mar 07 21:29:18 2014, NWELLNHOF wrote: Show quoted text

> On Thu Feb 13 10:52:47 2014, yoreek wrote:

> > Basically we can rewrite the source code so as not to pass object by > > $q->enqueue(@docs), but I can offer patch that adds check if variable > > is shared and if so, not to decrease REFCNT.

> > Unfortunately, this can't be made to work safely. Consider the > following sequence: > > my $doc = $parser->parse(...); > my $clone = shared_clone($doc); > $doc = undef; > print $clone->documentElement; > > When $doc is set to undef, it will be freed and subsequent usage via > the cloned SVs will fail (segfault or whatever). The way I see it, > it's simply impossible to make shared_clone work with XML::LibXML > objects. So I'd vote to revert your patch. > > But it should work to simply pass a reference to an XML::LibXML object > to $q->enqueue. > > $q->enqueue(map { \$_ } @docs);

Thanks Nick for the comment. I have tried to understand how works shared_clone and I think that shared_clone not work correctly with objects is a C structure: $make_shared = sub { my ($item, $cloned) = @_; ... # Copy a scalar ref elsif ($ref_type eq 'SCALAR') { $copy = \do{ my $scalar = $$item; }; ... bless($copy, $class); ... return $copy; }; In fact a reference to object is copied as number and blessed, the result is a new reference to same object, but not an increase REFCNT. If so, then we need fix shared_clone or modify code as suggested by Nick. Patch can be reverted because it only partially solves problem. -- YOREEK

Sat Mar 08 06:09:27 2014 NWELLNHOF [...] cpan.org - Correspondence added

On Sat Mar 08 04:25:28 2014, yoreek wrote: Show quoted text

> In fact a reference to object is copied as number and blessed, the > result is a new reference to same object, but not an increase REFCNT.

The Perl ref count is increased, but XML::LibXML has its own little objects (proxy nodes) which have another, separate ref count. The Perl objects contain a pointer to these proxy nodes. shared_clone copies this pointer without increasing the internal ref count. That's why you get the double free error. (The reason for the second reference count is that for every XML::LibXML::Node object, XML::LibXML has to keep a reference to the owner of that node, typically a document. Otherwise, the document could be destroyed prematurely.) Show quoted text

> If so, then we need fix shared_clone or modify code as suggested by > Nick. Patch can be reverted because it only partially solves problem.

I don't think shared_clone can be made to work with the kind of memory management that XML::LibXML uses.

Fri Sep 16 12:21:51 2016 jkahrman [...] mathworks.com - Correspondence added

From:

jkahrman [...] mathworks.com