Just a Theory: Defend Against Programmer Mistakes?

Defend Against Programmer Mistakes?

I get email:

Hey David,

I ran in to an issue earlier today in production that, while it is an error in my code, DBIx::Connector could easily handle the issue better. Here’s the use case:
package Con;
use Moose;
sub txn {
    my ($self, $code) = @_;
    my @ret;
    warn "BEGIN EVAL\n";
    eval{ @ret = $code->() };
    warn "END EVAL\n";
    die "DIE: $@" if $@;
    return @ret;
}
package main;
my $c = Con->new();
foreach (1..2) {
    $c->txn(sub{ next; });
}
The result of this is:
BEGIN EVAL
Exiting subroutine via next at test.pl line 16.
Exiting eval via next at test.pl line 16.
Exiting subroutine via next at test.pl line 16.
BEGIN EVAL
Exiting subroutine via next at test.pl line 16.
Exiting eval via next at test.pl line 16.
Exiting subroutine via next at test.pl line 16.
This means that any code after the eval block is not executed. And, in the case of DBIx::Connector, means the transaction is not commited or rolled back, and the next call to is txn() mysteriously combined with the previous txn() call. A quick fix for this is to just add a curly brace in to the eval:
eval{ { @ret = $code->() } };
Then the results are more what we’d expect:
BEGIN EVAL
Exiting subroutine via next at test.pl line 16.
END EVAL
BEGIN EVAL
Exiting subroutine via next at test.pl line 16.
END EVAL
I’ve fixed my code to use return; instead of next;, but I think this would be a useful fix for DBIx::Connector so that it doesn’t act in such an unexpected fashion when the developer accidentally calls next.

The fix here is pretty simple, but I’m not sure I want to get into the business of defending against programmer mistakes like this in DBIx::Connector or any module.

What do you think?

Tags:

Comments & Trackbacks

Abigail wrote:

Beside that such safety nets make sloppy programmers it might very well be that whatever safeguards you come up with it will actually hinder someone who uses something on purpose. That is, IMO, a high price to pay. Furthermore, where does one draw the line? Which errors are you going to write defences against? How time are you going to spend on it? Is it worth complicating your code for?

Matt S Trout wrote:

Don't punish cleverness - especially when 'use warnings' already caught the bug

Personally, I write "use warnings FATAL => 'all';" in all my code now, which means I'd just have got a test blowup here, rather than error log spew in production.

But either way, there was already a warning. Perl already told the developer there was something wrong. It ain't broke - no need to fix it.

Sam wrote:

Normally I'd agree and say "garbage in, garbage out", but in this case you're dealing with transactions: you're supposed to be robust in the face of garbage.

A transaction is supposed to succeed or rollback, there's no third option.

Now someone can always intentionally write something pathological, but accidentally leaving a next instead of a return when you're turning the code in a loop into a closure, well it happens, that's why there's a warning.

IMO no reasonable mistake should cause the "success or rollback" model of a transaction to break down, and ideally that promise should be as ironclad as possible.

Aran Deltac wrote:

Perl Hacker

I was the original author of this e-mail to David and I just want to chime in that what Sam just wrote about this fits exactly with my reasoning for suggesting that DBIx::Connector handle this case gracefully. Sam just said it more eloquently, thanks Sam.

Arthur Axel fREW Schmidt wrote:

Nah

I'd just put common missteps in the pod.

Theory wrote:

Went Ahead and Fixed It

Much as I am loathe to make my code try to work around someone else's mistakes, I went ahead and committed a fix for this particular issue.

Thanks, Sam, for your comment, which is what brought me around in this case. By returning from a function via next or last, a user was inadvertently exiting from the transactional methods (and perhaps the entire program!), thus skipping the transaction management code. The guarantee was getting violated, and I agree that it should do its best not to let that happen.

I just hope no one tries to return via exit, as they'll be screwed (any transactions will be rolled back, FWIW).

Thanks for the feedback, everyone.

—Theory

Discussion is now closed.

Just a Theory

Defend Against Programmer Mistakes?

Comments & Trackbacks

Don't punish cleverness - especially when 'use warnings' already caught the bug

Perl Hacker

Nah

Went Ahead and Fixed It

Syndicate

Where I work

Twitterstream

Stuff I Read

PGXN Code

CPAN Code

vCard