How to pass a file handle to a function?

Question

When I run the code below I get

Can't use string ("F") as a symbol ref while "strict refs" in use at ./T.pl line 21.

where line 21 is

flock($fh, LOCK_EX);

What am I doing wrong?

#!/usr/bin/perl

use strict;
use warnings;
use Fcntl ':flock', 'SEEK_SET'; # file locking
use Data::Dumper;
# use xx;

my $file = "T.yaml";
my $fh = "F";
my $obj = open_yaml_with_lock($file, $fh);

$obj->{a} = 1;

write_yaml_with_lock($obj, $fh);

sub open_yaml_with_lock {
    my ($file, $fh) = @_;

    open $fh, '+<', $file;
    flock($fh, LOCK_EX);
    my $obj = YAML::Syck::LoadFile($fh);

    return $obj;
}

sub write_yaml_with_lock {
    my ($obj, $fh) = @_;

    my $yaml = YAML::Syck::Dump($obj);
    $YAML::Syck::ImplicitUnicode = 1;
    seek $fh,0, SEEK_SET;   # seek back to the beginning of file

    print $fh $yaml . "---\n";
    close $fh;
}

Answer 1

What you're doing wrong is using the string "F" as a filehandle. This has never been something that's worked; you could use a bareword as a filehandle (open FH, ...; print FH ...), or you could pass in an empty scalar and perl would assign a new open file object to that variable. But if you pass in the string F, then you need to refer to then handle as F, not $fh. But, don't do that.

Do this instead:

sub open_yaml_with_lock {
    my ($file) = @_;

    open my $fh, '+<', $file or die $!;
    flock($fh, LOCK_EX) or die $!;

    my $obj = YAML::Syck::LoadFile($fh); # this dies on failure
    return ($obj, $fh);
}

We're doing several things here. One, we're not storing the filehandle in a global. Global state makes your program extremely difficult to understand -- I had a hard time with your 10 line post -- and should be avoided. Just return the filehandle, if you want to keep it around. Or, you can alias it like open does:

sub open_yaml_with_lock {
    open $_[0], '+<', $_[1] or die $!;
    ...
}

open_yaml_with_lock(my $fh, 'filename');
write_yaml_with_lock($fh);

But really, this is a mess. Put this stuff in an object. Make new open and lock the file. Add a write method. Done. Now you can reuse this code (and let others do the same) without worrying about getting something wrong. Less stress.

The other thing we're doing here is checking errors. Yup, disks can fail. Files can be typo'd. If you blissfully ignore the return value of open and flock, then your program may not be doing what you think it's doing. The file might not be opened. The file might not be locked properly. One day, your program is not going to work properly because you spelled "file" as "flie" and the file can't be opened. You will scratch your head for hours wondering what's going on. Eventually, you'll give up, go home, and try again later. This time, you won't typo the file name, and it will work. Several hours will have been wasted. You'll die several years earlier than you should because of the accumulated stress. So just use autodie or write or die $! after your system calls so that you get an error message when something goes wrong!

Your script would be correct if you wrote use autodie qw/open flock seek close/ at the top. (Actually, you should also check that "print" worked or use File::Slurp or syswrite, since autodie can't detect a failing print statement.)

So anyway, to summarize:

• Don't open $fh when $fh is defined. Write open my $fh to avoid thinking about this.

• Always check the return values of system calls. Make autodie do this for you.

• Don't keep global state. Don't write a bunch of functions that are meant to be used together but rely on implicit preconditions like an open file. If functions have preconditions, put them in a class and make the constructor satisfy the preconditions. This way, you can't accidentally write buggy code!

Update

OK, here's how to make this more OO. First we'll do "pure Perl" OO and then use Moose. Moose is what I would use for any real work; the "pure Perl" is just for the sake of making it easy to understand for someone new to both OO and Perl.

package LockedYAML;
use strict;
use warnings;

use Fcntl ':flock', 'SEEK_SET';
use YAML::Syck;

use autodie qw/open flock sysseek syswrite/;

sub new {
    my ($class, $filename) = @_;
    open my $fh, '+<', $filename;
    flock $fh, LOCK_EX;

    my $self = { obj => YAML::Syck::LoadFile($fh), fh => $fh };
    bless $self, $class;
    return $self;
}

sub object { $_[0]->{obj} }

sub write {
    my ($self, $obj) = @_;
    my $yaml = YAML::Syck::Dump($obj);

    local $YAML::Syck::ImplicitUnicode = 1; # ensure that this is
                                            # set for us only

    my $fh = $self->{fh};

    # use system seek/write to ensure this really does what we
    # mean.  optional.
    sysseek $fh, 0, SEEK_SET;
    syswrite $fh, $yaml;

    $self->{obj} = $obj; # to keep things consistent
}

Then, we can use the class in our main program:

use LockedYAML;

my $resource = LockedYAML->new('filename');
print "Our object looks like: ". Dumper($resource->object);

$resource->write({ new => 'stuff' });

Errors will throw exceptions, which can be handled with Try::Tiny, and the YAML file will stay locked as long as the instance exists. You can, of course, have many LockedYAML objects around at once, that's why we made it OO.

And finally, the Moose version:

package LockedYAML;
use Moose;

use autodie qw/flock sysseek syswrite/;

use MooseX::Types::Path::Class qw(File);

has 'file' => (
    is       => 'ro',
    isa      => File,
    handles  => ['open'],
    required => 1,
    coerce   => 1,
);

has 'fh' => (
    is         => 'ro',
    isa        => 'GlobRef',
    lazy_build => 1,
);

has 'obj' => (
    is         => 'rw',
    isa        => 'HashRef', # or ArrayRef or ArrayRef|HashRef, or whatever
    lazy_build => 1,
    trigger    => sub { shift->_update_obj(@_) },
);

sub _build_fh {
    my $self = shift;
    my $fh = $self->open('rw');
    flock $fh, LOCK_EX;
    return $fh;
}

sub _build_obj {
    my $self = shift;
    return YAML::Syck::LoadFile($self->fh);
}

sub _update_obj {
    my ($self, $new, $old) = @_;
    return unless $old; # only run if we are replacing something

    my $yaml = YAML::Syck::Dump($new);

    local $YAML::Syck::ImplicitUnicode = 1;

    my $fh = $self->fh;
    sysseek $fh, 0, SEEK_SET;
    syswrite $fh, $yaml;

    return;
}

This is used similarly:

 use LockedYAML;

 my $resource = LockedYAML->new( file => 'filename' );
 $resource->obj; # the object
 $resource->obj( { new => 'object' }); # automatically saved to disk

The Moose version is longer, but does a lot more runtime consistency checking and is easier to enhance. YMMV.