Bug #39360 for Net-DNS-ZoneFile-Fast: die() inside on

Wed Sep 17 12:01:40 2008 FANY [...] cpan.org - Ticket created

CC:	305045 [...] rt.noris.net
Subject:	die() inside on_error handler doesn't work

If I use a die() inside an on_error handler to parse(), virtually nothing happens, i. e. perl -MNet::DNS::ZoneFile::Fast -e 'Net::DNS::ZoneFile::Fast::parse( text => "broken", on_error => sub { die "@_" } )' won't give any output at all or stop program execution. The reason for this is that the error() sub-routine gets called inside the eval block originating from parse(), and since on_error turns the soft_error option on, parse() will only return undef. Attached is a (IMHO somewhat ugly but working) patch. If you do not plan to apply it or change this behaviour by other means, it should IMHO at least be documented. Regards, fany PS: The Changes file of Net::DNS::ZoneFile::Fast 1.0 only contains revisions up to 0.6.1.

Subject:

die.patch

--- /usr/local/lib/perl5/site_perl/5.10.0/Net/DNS/ZoneFile/Fast.pm 2008-05-26 19:14:06.000000000 +0200 +++ Net/DNS/ZoneFile/Fast.pm 2008-09-17 17:55:57.275241405 +0200 @@ -145,6 +145,7 @@ } }; if ($@) { + if ( $@ =~ /^USER ERROR: (.*)/s ) { die $1 } return undef if $param{soft_errors}; die; } @@ -205,6 +206,7 @@ sub error { if ($on_error) { + local $SIG{__DIE__} = sub { die "USER ERROR: @_" }; $on_error->($ln, @_); } else { warn "@_, line $ln\n" if $soft_errors && !$quiet;

Wed Sep 17 13:03:13 2008 wjhns117 [...] hardakers.net - Taken

Wed Sep 17 13:06:49 2008 wjhns117 [...] hardakers.net - Correspondence added

That looks like a reasonable solution to the problem except that it breaks the already existing soft_errors condition... How about this: we'll implement the change but also include an exception that it doesn't actually die if soft_errors is set (and maybe print a warning). Would that be sasifactory to you?

Wed Sep 17 13:06:51 2008 The RT System itself - Status changed from 'new' to 'open'

Wed Sep 17 13:36:34 2008 FANY [...] cpan.org - Correspondence added

RT-Send-CC:

305045 [...] rt.noris.net

First of all thanks for your very quick replies to this and my other reports regarding this module! On Mi. 17. Sep. 2008, 13:06:49, HARDAKER wrote: Show quoted text

> That looks like a reasonable solution to the problem except that it > breaks the already existing soft_errors condition...

In what way? IMHO soft_errors should only apply to errors thrown by the module itself. If I explicitly call die() within an on_error handler, I do expect the process to die. :-) Show quoted text

> How about this: we'll implement the change but also include an > exception that it doesn't actually die if soft_errors is set (and > maybe print a warning). Would that be sasifactory to you?

If there isn't something I still haven't caught on to about this, I'd still prefer either the suggested change or alternatively a documentation of this behaviour. But that's of course only MHO. Regards, fany

Wed Sep 17 16:20:28 2008 wjhns117 [...] hardakers.net - Correspondence added

Hmm... Mail failed to go through; using the web interface this time... There is a chance you'll get 2 copies of this. Show quoted text

FvR> If there isn't something I still haven't caught on to about this, I'd FvR> still prefer either the suggested change or alternatively a FvR> documentation of this behaviour. But that's of course only MHO.

Well, right now the 'soft_errors' means that any errors caught it tries to keep going (IE, I've seen people code on that assumption). What we really need is a 3-step process (ignore all, ignore some or ignore none) for ignoring errors instead of just the 2 we have now (ignore all or not). It's further complicated by dies/errors that can come from user functions are module functions. Actually, I've thought the state of soft_errors is broken in a number of places (multiple coders with multiple definitions of what they think a soft_error is doesn't help). EG, Consider the current code in the error() function; note that it prints a warning if soft_errors is set and then dies anyway. How about if we check the results of the on_error function (currently being ignored) and allow it to return "FAIL" or "CONTINUE" or something to that effect to provide signaling to the error() function itself? IE, I think this is the roughly the right code for a new error condition that allows for all 3 states: sub error { if ($on_error) { my $result = $on_error->($ln, @_); if ($result) { $globalerror = "$result"; die "$result"; } } elsif ($soft_errors) { warn "@_, line $ln\n" if $soft_errors && !$quiet; } else { die "@_, line $ln\n"; } } (and then catch $globalerror above, which gets around signal handling which seems slightly cleaner to avoid conflicts)

Fri Sep 19 08:27:03 2008 FANY [...] cpan.org - Correspondence added

RT-Send-CC:

305045 [...] rt.noris.net

On Mi. 17. Sep. 2008, 16:20:28, HARDAKER wrote: Show quoted text

> Well, right now the 'soft_errors' means that any errors caught it > tries to keep going

Well, it would still do that as long as the on_error handler does not raise an exception itself. And why would you want to do that if you want parse() to continue? Show quoted text

> Actually, I've thought the state of soft_errors is broken in a number > of places (multiple coders with multiple definitions of what they > think a soft_error is doesn't help). EG, Consider the current code > in the error() function; note that it prints a warning if soft_errors > is set and then dies anyway.

Er... yes, that's somewhat ugly. Show quoted text

> How about if we check the results of the on_error function (currently > being ignored) and allow it to return "FAIL" or "CONTINUE" or > something to that effect to provide signaling to the error() function > itself?

But just in order not to break backward compatibility, right? Show quoted text

> IE, I think this is the roughly the right code for a new error > condition that allows for all 3 states: > > > sub error > { > if ($on_error) { > my $result = $on_error->($ln, @_); > if ($result) { > $globalerror = "$result"; > die "$result";

But that would also break compatibility for existing on_error handlers which return a true value because it has never mattered so far. Show quoted text

> (and then catch $globalerror above, which gets around signal handling > which seems slightly cleaner to avoid conflicts)

If that's your concern, you could still do it this way: sub error { if ($on_error) { eval { $on_error->($ln, @_) }; die $globalerror = $@ if length $@; } [...] Regards, fany

Fri Oct 03 09:33:11 2008 wjhns117 [...] hardakers.net - Correspondence added

Ok, sorry for the delay. Got distracted by other things. Anyway, how about this for a final patch that I think is doing functionally what you want. I'll cave to the backwards-compat issue and let it go. Hopefully no one is using a die for a soft fail case in any existing on_error callbacks. The code is functionally the same as your last code, but expanded a bit for more novice perl readers (not that most of the rest of the code is suitable for novice perl readers). Let me know if you think this'll do:

Index: Fast.pm =================================================================== --- Fast.pm (revision 4238) +++ Fast.pm (working copy) @@ -76,6 +76,7 @@ my @fhs; my @lns; my $includes_root; +my $globalerror; # boot strap optional DNSSEC module functcions # (not optional if trying to parse a signed zone, but we don't need @@ -146,6 +147,7 @@ } }; if ($@) { + die "$globalerror (at input line #$ln)" if ($globalerror); return undef if $param{soft_errors}; die; } @@ -206,7 +208,12 @@ sub error { if ($on_error) { - $on_error->($ln, @_); + eval { $on_error->($ln, @_) }; + if(defined($@)) { + # set global error so parse can die appropriately later. + $globalerror = $@; + die; + } } else { warn "@_, line $ln\n" if $soft_errors && !$quiet; }

Mon Oct 06 08:31:54 2008 fany [...] noris.net - Correspondence added

CC:	FANY [...] cpan.org, 305045 [...] rt.noris.net
Subject:	Re: [rt.cpan.org #39360] die() inside on_error handler doesn't work
Date:	Mon, 6 Oct 2008 14:31:28 +0200
To:	Wes Hardaker via RT <bug-Net-DNS-ZoneFile-Fast [...] rt.cpan.org>
From:	"Martin H. Sluka" <fany [...] noris.net>

Show quoted text

> The code is functionally the same as your last code, but expanded a bit > for more novice perl readers (not that most of the rest of the code is > suitable for novice perl readers). Let me know if you think this'll do:

Show quoted text

> + if(defined($@)) {

You should really test "length($@)" or "$@ ne ''" here, because $@ is defined even if everything went well. Apart from that, I think this would be a good solution. Regards, fany

Tue Oct 07 13:04:42 2008 wjhns117 [...] hardakers.net - Correspondence added

On Mon Oct 06 08:31:54 2008, fany@noris.net wrote: Show quoted text

> You should really test "length($@)" or "$@ ne ''" here, > because $@ is defined even if everything went well.

How very un-perl like to define something not yet in use. Thanks for the warning.

Tue Oct 07 13:29:35 2008 wjhns117 [...] hardakers.net - Correspondence added

Resolved in just released 1.01

Tue Oct 07 13:29:37 2008 wjhns117 [...] hardakers.net - Status changed from 'open' to 'resolved'

Bug #39360 for Net-DNS-ZoneFile-Fast: die() inside on_error handler doesn't work