Painful, painful bug

Ovid on 2008-06-24T16:58:24

jplindstrom asked me if I had run the tests since they were failing. Of course I ran them! I had no idea how they were failing now when they passed a few minutes earlier. However, with lots to do that day, I forgot about it. Then today it happened again. There were some tests which failed when run individually, but were passing when run via Test::Aggregate. I was worried that there was a bug in my module, but it turns out to be a real bug which was spotted:

Here was my test case:

  use PIPTest::Setup;
  eval {
      my $setup  = PIPTest::Setup->new;
      my $schema = $setup->init_db;
      die "ha!";
  };
  print "\$\@ is $@\n";

That printed "$@ is". Something about the &init_db call was screwing up $@. After a binary search through the code, I got it down to this:

    my $schema = $self->connect_to_db();

    # die "We see this in $@";
    $self->static_table_count($count);
    die "We don't see this in $@";

So what was in that mysterious &static_data_count method? It's from Class::Accessor::Fast:

sub static_table_count {
    return $_[0]->{static_table_count} if @_ == 1;
    return $_[0]->{static_table_count} = $_[1] if @_ == 2;
}

I copied that directly into my code and yup, if that got called, it appeared to break eval somehow. I was completely stumped. Three of us wound up pouring over that code until the obvious question was asked: "do we have a DESTROY method?"

Well, duh. In the DESTROY, we have dbic code. Naturally, that uses evals internally and when our code dies, the DESTROY is triggered, wiping out my $@. Adding local $@ at the top of the DESTROY fixed that.

So please, please, please: if you use any of Perl's internal variable, localize it first.

Perl::Critic

ChrisDolan on 2008-06-25T03:46:35

Perl::Critic::Policy::Variables::RequireLocalizedPunctuationVars to the rescue!

Re:Perl::Critic

jplindstrom on 2008-06-25T13:09:15

That's not quite what this is about. The problem is that $@ gets set to undef whenever an eval runs successfully, eradicating the current exception object before it can get handled by reaching the intended eval.

The critic rule would be to always require

DESTROY {
    local $@;
    ...
}

where ... either contains an eval BLOCK, or a sub call which could potentially involve an eval BLOCK. A bit pedantic perhaps, but probably worth it since the alternative is to risk silencing exceptions!.

Of course other global vars could be problematic in the same way, but $@ is in an especially weak position since it's involved in flow control.