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.

Answer 2

Replace

my $fh = "F"; # text and also a ref in nonstrict mode

with

my $fh = \*F; # a reference, period

Of course, it's better yet to use lexical filehandles, as in open my $fd, ... or die ..., but that's not always possible, e.g. you have STDIN that's predefined. In such cases, use \*FD wherever $fd fits.

There's also a case with old scripts, you have to watch out where a global FD is opened and closed.

Answer 3

If you use the value directly in the sub, it will work:

use strict;
use warnings;
use autodie;

my $fh;
yada($fh);
print $fh "testing, testing";

sub yada {
    open $_[0], '>', 'yada.gg';
}

Or as a reference:

yada(\$fh);

sub yada {
    my $handle = shift;
    open $$handle, '>', 'yada.gg';
}

Or better yet, return a filehandle:

my $fh = yada($file);

sub yada {
    my $inputfile = shift;
    open my $gg, '>', $inputfile;
    return $gg;
}

Answer 4

From the documentation:

 open FILEHANDLE,EXPR

If FILEHANDLE is an undefined scalar variable (or array or hash element) the variable is assigned a reference to a new anonymous filehandle, otherwise if FILEHANDLE is an expression, its value is used as the name of the real filehandle wanted. (This is considered a symbolic reference, so "use strict 'refs'" should not be in effect.)

Filehandle here is an expression ("F") so itsvalue is used as the name of the real filehandle you want. (A filehandle called F). And then... the documentation says "use strict 'refs'" should not be in effect, because you're using F as a symbolic reference.

(use strict; on line 1 includes strict 'refs'.)

Had you just said at the beginning:

  my $fh;

This would have worked, because then $fh would become a reference to a new anonymous filehandle and Perl won't try to use it as a symbolic reference.

This works:

#!/usr/bin/perl

my $global_fh;

open_filehandle(\$global_fh);
use_filehandle(\$global_fh);

sub open_filehandle {
    my ($fh)=@_;

    open($$fh, ">c:\\temp\\testfile") || die;
}

sub use_filehandle {
    my($fh) = @_;

    # Print is pecular that it expects the next token to be the filehandle
    # or a simple scalar.  Thus, print $$fh "Hello, world!" will not work.
    my $lfh = $$fh;
    print $lfh "Hello, world!";   

    close($$fh);
}

Or you can do what the other poster suggested and use $_[1] directly, but that's a bit harder to read.