only deserialize on non-empty $buffer

[martin-rueegg]

According to the documentation, the run function does only require the first argument to be present:

Takes a code reference (required) which is the job to be run on the child. If the next argument is an array reference, these will be passed to the child job. If the last argument is a code reference, it will be called immediately before the finish event is emitted, its arguments are the same as the finish event.

In order to receive system signals, e.g. $SIG(INT), at any time (if only timers are running during the IOLoop, signals will only be caught and handled after the first timer is fired), I created the following "ghost" process, that will do nothing, but just be there. This enables signals to be caught at any time.

my $heartbeat = sub {
    my $self = shift;

    my $fc = Mojo::IOLoop::ForkCall->new;
    $fc->run(
        sub {
            $self->zLog->debug('Heartbeat started.'); # $self->zLog is a Mojo::Log
            sleep
        }
    );
};

however, when sending a SIGTERM to the perl process, the following error is thrown at the very end:

    (in cleanup) Mojo::IOLoop::ForkCall: Magic number checking on storable string failed at /usr/lib/perl/5.18/Storable.pm line 417, at /opt/znapzend-0.15.2/lib/Mojo/IOLoop/ForkCall.pm line 95.

The condition included in this PR, that the $buffer will only be de-serialized if it is non-empty, resolves the issue. Both cases (error and no-error) are reproducible.

[jberger]

In order to accept this PR I'd like to see a test case and I think we should decide on what the module SHOULD do when it has an empty buffer. Sure I probably shouldn't try to deserialize it but I also don't think it should just silently do nothing. I can imagine other failure scenarios where this behavior would but a regression.

[jberger]

I could POSSIBLY be convinced that if the buffer is empty AND we are in global destruction then perhaps it could silently do nothing.

[martin-rueegg]

The serialization and deserialization is regarding the function arguments.

1. Line35: $args remains undef in case there are no arguments.
2. Storable->_freeze returns the result from C routine mstore or undef: return $ret ? $ret : undef;
3. Lines 55 and 59 both set the result from $serializer and thus from Storable::freeze (possibly undef).
4. It is then sent through the pipely from the child to the parent
5. the parent receives the data in Line 79 and concats it to the buffer: $buffer .= $_[1]. Due to concatination $buffer remains "" (see test script)
6. in line 94 the data is sent to Storable::thaw which in turn may return undef
7. however, as the following script shows, $buffer is always one of the following:
   1. '' in case the on read action would return undef
   2. a non-empty string with length($buffer) >= 21

use Storable;
use Data::Dumper;

my $buffer = '';
$buffer .= undef;

sub d {
  my $arg = shift;
  if (defined $arg) {
        print "defined\n";
  } else {
        print "undefined\n";
  };
}

print "buffer concated with undef:\n";
d $buffer;
print 'length($buffer)=' . length($buffer) . "\n\n";
print "undef:\n";
d undef;

my $data = "this is a string of text";
my @dataset = qw( this is an array of text );
my %datagroup = ( hash => "mine", text => "yours" );

sub s_d {
        my @args = @_;
        my $args = \@args;
        print "----------\n";
        print "10: ".Dumper(@args);
        print "----------\n";
        my $buffer = Storable::freeze [ @args ];
        print "20: ".Dumper($buffer);
        print "21: ".length($buffer)."\n";
        print "----------\n";
        my $res = Storable::thaw $buffer;
        d $res;
        print "30: ".Dumper($res);
        print "\n";
}

print "\n\nundef\n";
s_d undef;

print "\n\n''\n";
s_d '';

print "\n\n0\n";
s_d 0;

print "\n\n$data\n";
s_d $data;

print "\n\ncomplex\n";
s_d [ \$data, \@dataset, \%datagroup ];

so according to this, both if $buffer and if length($buffer); would work.

btw, it is not about silently doing nothing. just not deserializing, since there are no $args in the first place. the callback and the finish event will still be executed.

[jberger]

Lines 55 and 59 both set the result from $serializer and thus from Storable::freeze (possibly undef).

this isn't true. If you reread you will see that the arguments to be returned are in both cases wrapped in an array reference. The reason for this is so that ...

in line 94 the data is sent to Storable::thaw which in turn may return undef

this isn't true either. The result of the deserialization is always an array reference because I ensured that before. Line 101 relies on that fact. If you were to leave $res undefined that line will complain about not having an array reference to dereference.

[martin-rueegg]

Lines 55 and 59 both set the result from $serializer and thus from Storable::freeze (possibly undef).

this isn't true. If you reread you will see that the arguments to be returned are in both cases wrapped in an array reference. The reason for this is so that ...

as i said, i'm not so familiar with perl language. i just assume the following:

1. if the code is my $res = eval {$serializer->([undef, $job->(@$args)]);};
2. then $res will be set to the result of whatever eval {} returns
3. eval returns the result of the last statement, so $serializer->([undef, $job->(@$args)]);
4. $deserializer returns what Storable::freeze returns
5. freeze returns the result of _freeze(\&mstore, @_)
6. ... which in turn returns return $ret ? $ret : undef;
7. and as such "possibly undef"
8. $ret is $ret = &$xsptr($self), so in this case the result of mstore($self) where $self is the array [undef, $job->(@$args)]
9. so unless mstore doesn't throw an error, you're right, there should not be an undef

for the deserialization, it's similar: my $res = eval { $deserializer->($buffer) } will return an array, as long as there was no error in the serialization.

thus we should ensure that $res is at least an empty array if we do not deserialize.

something along the following lines might do the trick:

my $res = ();
if (length($buffer)) {
  $res = eval { $deserializer->($buffer) };
  if ($@) { 
    $self->emit( error => $@ ) if $self;
    return;
  }
}

what do you think?

[martin-rueegg]

have updated the change accordingly.

[jberger]

I'm sorry but that change still just silently continues without any value for $res if there was no length of the buffer. In cases where the socket died for example that is clearly an error and shouldn't be ignored

[martin-rueegg]

In cases where the socket died for example that is clearly an error and shouldn't be ignored

isn't that what $stream->on( error => sub { $self->emit( error => $_[1] ) if $self } ); is for?