| Message ID | 1450371968-27997-6-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote: > This warning appears when db_retry_abort is used, since 2b069b6c > "Database locking: Perl: Retry all deadlocks in PostgreSQL". Is the reason for not turning this into a return related to the fact this is within an eval, not a proper sub? But then I'm not really sure why it is warning (I suppose eval has some sub like properties?). Related to https://rt.perl.org/Public/Bug/Display.html?id=29238 perhaps. > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > --- > Osstest.pm | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Osstest.pm b/Osstest.pm > index c77d960..a398b1e 100644 > --- a/Osstest.pm > +++ b/Osstest.pm > @@ -299,6 +299,7 @@ sub db_retry ($$$;$$) { > $r= &$body; > if ($db_retry_stop) { > $dbh->rollback(); > + no warnings qw(exiting); > last if $db_retry_stop eq 'abort'; > next; > }
Ian Campbell writes ("Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning"):
> On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> > This warning appears when db_retry_abort is used, since 2b069b6c
> > "Database locking: Perl: Retry all deadlocks in PostgreSQL".
>
> Is the reason for not turning this into a return related to the fact this
> is within an eval, not a proper sub? But then I'm not really sure why it is
> warning (I suppose eval has some sub like properties?).
I think it's a warning because eval does not trap `last's (nor
`return's). This means that if you think `eval' can be used to make
try-finally, you will be disappointed.
(Unlike, in Tcl, say, where `break' and `continue' and indeed `return'
are implemented as exceptions.)
Why would `return' be better ? It would still generate the same
warning. Leaving the loop via `last' seems less heavyweight than
using `return' and less likely to produce bugs in future patches whose
authors don't spot the non-local exit.
> Related to https://rt.perl.org/Public/Bug/Display.html?id=29238 perhaps.
I don't think so, although there seems to be a fair amount of
confusion there.
Ian.
On Thu, 2015-12-17 at 17:48 +0000, Ian Jackson wrote: > Ian Campbell writes ("Re: [OSSTEST PATCH 6/9] db_retry: Suppress an > "exiting via last" warning"): > > On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote: > > > This warning appears when db_retry_abort is used, since 2b069b6c > > > "Database locking: Perl: Retry all deadlocks in PostgreSQL". > > > > Is the reason for not turning this into a return related to the > fact this > > is within an eval, not a proper sub? But then I'm not really sure > why it is > > warning (I suppose eval has some sub like properties?). > > I think it's a warning because eval does not trap `last's (nor > `return's). This means that if you think `eval' can be used to make > try-finally, you will be disappointed. > > (Unlike, in Tcl, say, where `break' and `continue' and indeed > `return > are implemented as exceptions.) > > Why would `return' be better ? It would still generate the same > warning. Leaving the loop via `last' seems less heavyweight than > using `return' and less likely to produce bugs in future patches whose > authors don't spot the non-local exit. I think I'm probably confused about the scope of the eval vs {last,return} etc. In any case, that's no reason to block this patch AFAICT: Acked-by: Ian Campbell <ian.campbell@citrix.com> Ian. > > > Related to > https://rt.perl.org/Public/Bug/Display.html?id=29238 perhaps. > > I don't think so, although there seems to be a fair amount of > confusion there. > > Ian.
Ian Campbell writes ("Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning"):
> On Thu, 2015-12-17 at 17:48 +0000, Ian Jackson wrote:
> > Why would `return' be better ? It would still generate the same
> > warning. Leaving the loop via `last' seems less heavyweight than
> > using `return' and less likely to produce bugs in future patches whose
> > authors don't spot the non-local exit.
>
> I think I'm probably confused about the scope of the eval vs
> {last,return} etc. In any case, that's no reason to block this patch
> AFAICT: Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks.
FYI, it's (eliding lots) like this:
sub db_retry ($$$;$$) {
for (;;) {
$pre->();
eval {
$r= &$body;
if ($db_retry_stop) {
no warnings qw(exiting);
last if $db_retry_stop eq 'abort';
next;
}
$dbh->commit();
};
last if !length $@;
}
return $r;
}
The warning from perl is because when `last if $db_retry_stop'
happens, `last' exits the eval in order to exit the outer for (;;)
loop.
The same would apply for the `next'. If there were any callers of
db_retry_retry, they would trigger the `next', which would exit the
eval in order to restart the outer for.
Both of these are intentional. Perl thinks they might not be.
Thanks for all the acks. I'll maybe push this at some point, or
combine it with something else. It's not urgent.
Ian.
On Thu, 2015-12-17 at 18:38 +0000, Ian Jackson wrote: Continuing purely for my own edification... > FYI, it's (eliding lots) like this: > > sub db_retry ($$$;$$) { > for (;;) { > $pre->(); > eval { > $r= &$body; > if ($db_retry_stop) { > no warnings qw(exiting); > last if $db_retry_stop eq 'abort'; > next; > } > $dbh->commit(); > }; > last if !length $@; > } > return $r; > } > > The warning from perl is because when `last if $db_retry_stop' > happens, `last' exits the eval in order to exit the outer for (;;) > loop. Meaning that last (or next) will exit the eval and immediately end (or restart) the containing loop, in particular without executing the "last if !length $@;". > Both of these are intentional. Perl thinks they might not be. ... and that's because it is a common enough mistake to think that next or last exits only the scope of the eval and therefore in this case to expect it to run "last if !length $@;", while it actually wont. I would have expected that if you want to successfully exit (i.e. not die) the eval early and continue on with the remainder of the loop you should use return, and http://perldoc.perl.org/functions/eval.html seems to back that up ("the value returned is the value of the last expression evaluated inside the mini-program; a return statement may be also used,"), but earlier you said that return would generate the same warning, so I remain a bit confused. According to my reading of eval.html "return if $db_retry_stop eq 'abort'" would exit the eval with $@="" ("If there was no error, $@ is set to the empty string", it mentions last and goto bypassing this, but not return). Then since length "" is false that outer last would be run insteadand we are done with the loop as we wanted. AFAICT that would avoid the warning for the inner last, but is not usable for the inner next, and in any case using last inside the eval is actually more obvious in this instance, once one thinks one understands an eval (which in my case is still debatable I think) Ian. > Thanks for all the acks. I'll maybe push this at some point, or > combine it with something else. It's not urgent. > > Ian.
Ian Campbell writes ("Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning"):
> On Thu, 2015-12-17 at 18:38 +0000, Ian Jackson wrote:
> > Both of these are intentional. Perl thinks they might not be.
>
> ... and that's because it is a common enough mistake to think that next or
> last exits only the scope of the eval and therefore in this case to expect
> it to run "last if !length $@;", while it actually wont.
Yes.
> I would have expected that if you want to successfully exit (i.e. not die)
> the eval early and continue on with the remainder of the loop you should
> use return, and http://perldoc.perl.org/functions/eval.html seems to back
> that up ("the value returned is the value of the last expression evaluated
> inside the mini-program; a return statement may be also used,"), but
> earlier you said that return would generate the same warning, so I remain a
> bit confused.
Oh! I was confused about the effect of `return' in an eval. I
expected it to exit past the eval and return out of the calling
function.
> According to my reading of eval.html "return if $db_retry_stop eq 'abort'"
> would exit the eval with $@="" ("If there was no error, $@ is set to the
> empty string", it mentions last and goto bypassing this, but not return).
> Then since length "" is false that outer last would be run insteadand we
> are done with the loop as we wanted. AFAICT that would avoid the warning
> for the inner last, but is not usable for the inner next, and in any case
> using last inside the eval is actually more obvious in this instance, once
> one thinks one understands an eval (which in my case is still debatable I
> think)
I still think this reasoning about `return' is more fiddly than just
saying `last' or `next' when we mean `last' or `next'...
Particularly given that, now, there is a `no warnings qw(exiting)' to
cue the reader that they may need to check the FM.
Ian.
diff --git a/Osstest.pm b/Osstest.pm index c77d960..a398b1e 100644 --- a/Osstest.pm +++ b/Osstest.pm @@ -299,6 +299,7 @@ sub db_retry ($$$;$$) { $r= &$body; if ($db_retry_stop) { $dbh->rollback(); + no warnings qw(exiting); last if $db_retry_stop eq 'abort'; next; }
This warning appears when db_retry_abort is used, since 2b069b6c "Database locking: Perl: Retry all deadlocks in PostgreSQL". Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> --- Osstest.pm | 1 + 1 file changed, 1 insertion(+)