Bug #32689 for KinoSearch: KinoSearch::Simple and DESTROY

Sat Jan 26 16:08:45 2008 sprout [...] cpan.org - Ticket created

Subject:	KinoSearch::Simple and DESTROY
Date:	Sat, 26 Jan 2008 13:08:23 -0800
To:	bug-kinosearch [...] rt.cpan.org
From:	Father Chrysostomos <sprout [...] cpan.org>

I found a bug in KinoSearch::Simple 0.02_051 which I believe is still present in the svn repo (rev. 2955). The DESTROY method relies on another object’s (the indexer’s) existence. If the KS:Simple object is stored in a global variable, or a lexical variable visible to a package sub, etc., it is not destroyed until the program quits. During global destruction, the indexer may get destroyed first, so an unfinished (and unusable) index is created. The attached patch should fix this. Is there any reason why InvIndexer’s NUKE method should not call finish_indexing anyway? If not, then the fix can be even simpler than this patch.

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

Wed Jan 30 23:52:10 2008 CREAMYG [...] cpan.org - Correspondence added

If the InvIndexer object can get destroyed before the KS::Simple object that contains it, then can't the InvIndexer's interior members get destroyed before the InvIndexer that contains them? And if that's the case, then wouldn't finish_on_destroy have to propagate to all of them?

Wed Jan 30 23:52:11 2008 The RT System itself - Status changed from 'new' to 'open'

Fri Feb 01 21:04:07 2008 sprout [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #32689] KinoSearch::Simple and DESTROY
Date:	Fri, 1 Feb 2008 18:03:34 -0800
To:	bug-KinoSearch [...] rt.cpan.org
From:	Father Chrysostomos <sprout [...] cpan.org>

On Jan 30, 2008, at 8:52 PM, Marvin Humphrey via RT wrote: Show quoted text

> > <URL: http://rt.cpan.org/Ticket/Display.html?id=32689 > > > If the InvIndexer object can get destroyed before the KS::Simple > object > that contains it, then can't the InvIndexer's interior members get > destroyed before the InvIndexer that contains them? And if that's the > case, then wouldn't finish_on_destroy have to propagate to all of > them?

Hmm, I suppose you’re right. Is there any reason that this should not be the default behaviour? The objects could automatically finish the index upon destruction. But if this is complicated, considering how easy a workaround is (undef $ks) once one understands the problem, maybe it’s not worth the time. We could instead put a caveat in KinoSearch::Simple’s documentation, especially considering how easy it is to trigger: my $ks = KinoSearch::Simple->new( ... ); index_page($first_page); send_email_notification(); sub index_page { ... $ks->add_doc( ... ); index_page( $foo ) if $bar; } In this example, the $ks is referred to inside index_page, which is in the symbol table; hence $ks is not destroyed until program exit.

Wed Feb 20 22:01:26 2008 CREAMYG [...] cpan.org - Correspondence added

From:

CREAMYG [...] cpan.org

Show quoted text

> > If the InvIndexer object can get destroyed before the KS::Simple > > object > > that contains it, then can't the InvIndexer's interior members get > > destroyed before the InvIndexer that contains them? And if that's > > the case, then wouldn't finish_on_destroy have to propagate to all > > of them?

As documented in the message I recently sent to the KinoSearch discussion list, I'm no longer worried about that problem. Show quoted text

> Is there any reason that this should not > be the default behaviour? The objects could automatically finish the > index upon destruction.

For core KinoSearch, it is important to give users maximum control over commit actions. The automatic finishing should be unique to Simple. Show quoted text

> But if this is complicated, considering how easy a workaround is > (undef $ks) once one understands the problem, maybe it’s not worth the > time. We could instead put a caveat in KinoSearch::Simple’s > documentation,

I think that's harder to grok than I'd like -- I don't want Simple users to have to think about DESTROY. If we can't fix this transparently, I'd prefer to either commit on every call to add_doc() (expensive), or add an explicit finish() method (verbose). However, I wonder if we can't solve this with an END block: register all objects created by Simple->new, and have all the registered objects finish() during the END block, before global destruction commences.

Thu Feb 28 12:40:17 2008 sprout [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #32689] KinoSearch::Simple and DESTROY
Date:	Thu, 28 Feb 2008 09:30:18 -0800
To:	bug-KinoSearch [...] rt.cpan.org
From:	Father Chrysostomos <sprout [...] cpan.org>

On Feb 20, 2008, at 7:01 PM, Marvin Humphrey via RT wrote: Show quoted text

> However, I wonder if we can't solve this with an END block: register > all > objects created by Simple->new, and have all the registered objects > finish() during the END block, before global destruction commences.

Here’s a patch that does just this. I’ve not tested it at all, though. In writing tests, I need to run another perl process, though I’m not sure what is the best way to do this. Will ‘system $^X => @args’ be sufficient, or is there something I’m not taking into account?

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

Thu Feb 28 14:34:57 2008 CREAMYG [...] cpan.org - Correspondence added

From:

CREAMYG [...] cpan.org

Show quoted text

> In writing tests, I need to run another perl process, though I’m not > sure what is the best way to do this. Will ‘system $^X => @args’ be > sufficient, or is there something I’m not taking into account?

There are a couple tests in the KS suite that use fork(), and that mechanism should work here as well -- take a look at t/106-locking.t. If we create the Simple object and perform indexing in the child process, we can verify success via the parent process once the child exits. The patch looks good to me, but I'll wait to apply it until we have a test. :)

Fri Feb 29 11:31:41 2008 sprout [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #32689] KinoSearch::Simple and DESTROY
Date:	Fri, 29 Feb 2008 08:31:02 -0800
To:	bug-KinoSearch [...] rt.cpan.org
From:	Father Chrysostomos <sprout [...] cpan.org>

On Feb 28, 2008, at 11:34 AM, Marvin Humphrey via RT wrote: Show quoted text

> The patch looks good to me, but I'll wait to apply it until we have a > test. :)

A good thing, because my patch was flawed. Here’s a patch complete with tests and all:

On Feb 28, 2008, at 11:34 AM, Marvin Humphrey via RT wrote:

Show quoted text

The patch looks good to me, but I'll wait to apply it until we have a
test. :)

A good thing, because my patch was flawed. Here’s a patch complete with tests and all:

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

Fri Feb 29 18:20:28 2008 CREAMYG [...] cpan.org - Fixed in 0.20_05 added

Fri Feb 29 18:20:29 2008 CREAMYG [...] cpan.org - Correspondence added

Thanks for the test and the patch. I have indeed been able to duplicated the problem on my system by pulling out the END block. Looks like a proper fix! Applied as r3089.

Fri Feb 29 18:20:30 2008 CREAMYG [...] cpan.org - Status changed from 'open' to 'resolved'