Skip Menu |
 

This queue is for tickets about the Net-DNS-ZoneFile-Fast CPAN distribution.

Report information
The Basics
Id: 39360
Status: resolved
Priority: 0/
Queue: Net-DNS-ZoneFile-Fast

People
Owner: wjhns117 [...] hardakers.net
Requestors: FANY [...] cpan.org
Cc: 305045 [...] rt.noris.net
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.0
Fixed in: (no value)



CC: 305045 [...] rt.noris.net
Subject: die() inside on_error handler doesn't work
Download (untitled) / with headers
text/plain 738b
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
Download die.patch
text/x-diff 555b
--- /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;
Download (untitled) / with headers
text/plain 309b
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?
RT-Send-CC: 305045 [...] rt.noris.net
Download (untitled) / with headers
text/plain 861b
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
Download (untitled) / with headers
text/plain 1.7k
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)
RT-Send-CC: 305045 [...] rt.noris.net
Download (untitled) / with headers
text/plain 1.6k
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
Download (untitled) / with headers
text/plain 514b
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:
Download fast.patch
text/x-diff 842b
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; }
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>
Download (untitled) / with headers
text/plain 428b
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
Download (untitled) / with headers
text/plain 247b
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.
Resolved in just released 1.01


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.