What should I do if a Moose builder method fails?

Question

What is the best way to handle a failure in a builder method?

For example:

package MyObj;
use Moose;
use IO::File;

has => 'file_name'   ( is => 'ro', isa => 'Str',      required   =>1  );
has => 'file_handle' ( is => 'ro', isa => 'IO::File', lazy_build => 1 );

sub _build_file_handle {
    my $self = shift;
    my $fh = IO::File->new( $self->file_name, '<' );

    return $fh;
}

If the _build_file_handle fails to get a handle, the builder will return undef, which fails the type constraint.

I could use a union in the file_handle type constraint, so that it will accept an undef as a valid value. But then, the predicate has_file_handle will return true, even when the value is undef.

Is there a way to signal that the builder failed, and the attribute should remain cleared?

Answer 1

You are not thinking at a high-enough level. OK, the builder fails. The attribute remains undefined. But what do you do about the code that is calling the accessor? The class' contract indicated that calling the method would always return an IO::File. But now it's returning undef. (The contract was IO::File, not Maybe[IO::File], right?)

So on the next line of code, the caller is going to die ("Can't call method 'readline' on an undefined value at the_caller.pl line 42."), because it expects your class to follow the contract that it defined. Failure was not something your class was supposed to do, but now it did. How can the caller do anything to correct this problem?

If it can handle undef, the caller didn't actually need a filehandle to begin with... so why did it ask your object for one?

With that in mind, the only sane solution is to die. You can't meet the contract you agreed to, and die is the only way you can get out of that situation. So just do that; death is a fact of life.

Now, if you aren't prepared to die when the builder runs, you'll need to change when the code that can fail runs. You can do it at object-construction time, either by making it non-lazy, or by explicitly vivifying the attribute in BUILD (BUILD { $self->file_name }).

A better option is to not expose the file handle to the outside world at all, and instead do something like:

# dies when it can't write to the log file
method write_log {
    use autodie ':file'; # you want "say" to die when the disk runs out of space, right?
    my $fh = $self->file_handle;
    say {$fh} $_ for $self->log_messages;
}

Now you know when the program is going to die; in new, or in write_log. You know because the docs say so.

The second way makes your code a lot cleaner; the consumer doesn't need to know about the implementation of your class, it just needs to know that it can tell it to write some log messages. Now the caller is not concerned with your implementation details; it just tells the class what it really wanted it to do.

And, dying in write_log might even be something you can recover from (in a catch block), whereas "couldn't open this random opaque thing you shouldn't know about anyway" is much harder for the caller to recover from.

Basically, design your code sanely, and exceptions are the only answer.

(I don't get the whole "they are a kludge" anyway. They work the exact same way in C++ and very similarly in Java and Haskell and every other language. Is the word die really that scary or something?)

Answer 2

Is there a way to signal that the builder failed, and the attribute should remain cleared?

No. This wouldn't make sense, the builder will fire if the attribute is cleaered, if it was cleared within the builder it would just fire when you made the next call to it, and remain in the cleared state. A waste of a lot work, just to set-something-if-it-works-and-if-not-continue.

The type-union suggestion is a good one but then you have to write code that can function with two radically different cases: a filehandle, and a non-existant file handle. This seems like a poor idea.

If the file-handle isn't essential to the task then it probably isn't shared amongst the same scope of things with access to the object. If this is the case then the object can just provide a method that generates a filehandle from the object. I do this in production code. Don't get hellbent on making everything a lazy-attribute, some things are functions of attribute and it doesn't always make sense to attach them to the object.

sub get_fh {                                                                
  my $self = shift;                                                         

  my $abs_loc = $self->abs_loc;                                             

  if ( !(-e $abs_loc) || -e -z $abs_loc ) {                                 
    $self->error({ msg => "Critical doesn't exist or is totally empty" });  
    die "Will not run this, see above error\n";                             
  }                                                                         

  my $st = File::stat::stat($abs_loc);                                      
  $self->report_datetime( DateTime->from_epoch( epoch => $st->mtime ) );    

  my $fh = IO::File->new( $abs_loc, 'r' )                                   
    || die "Can not open $abs_loc : $!\n"                                   
  ;                                                                         

  $fh;                                                                      

}                                                                           

A totally different approach is to subclass IO::File, with the meta data about the file you want to keep. Sometimes this is effective, and a great solution:

package DM::IO::File::InsideOut;
use feature ':5.10';
use strict;
use warnings;

use base 'IO::File';

my %data;

sub previouslyCreated {
  $data{+shift}->{existed_when_opened}
}

sub originalLoc {
  $data{+shift}->{original_location}
}

sub new {
  my ( $class, @args ) = @_;

  my $exists = -e $args[0] ? 1 : 0;

  my $self = $class->SUPER::new( @args );

  $data{$self} = {
    existed_when_opened => $exists
    , original_location => $args[0]
  };

  $self;

};

Answer 3

"Best" is subjective, but you'll have to decide which makes more sense in your code:

1. if you can continue on in your code when the filehandle fails to build (i.e. it is a recoverable condition), the builder should return undef and set the type constraint to 'Maybe[IO::File]'. That means you'll also have to check for definedness on that attribute whenever using it. You could also check if this attribute got built properly in BUILD, and choose to take further action at that point (as friedo alluded to in his comment), e.g. calling clear_file_handle if it is undef (since a builder will always assign a value to the attribute, assuming it doesn't die of course).

2. otherwise, let the builder fail, either by explicitly throwing an exception (which you can opt to catch higher up), or simply returning undef and letting the type constraint fail. Either way your code will die; you just get a choice of how it dies and how voluminous the stack trace is. :)

PS. You may also want to look at Try::Tiny, which Moose uses internally, and is basically just a wrapper for* the do eval { blah } or die ... idiom.

*_{But done right! and in a cool way! (I seem to hear lots of whispering in my ear from #moose..)}