perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

Unexpected object type (8) in store_hook()

Open jackdeguest opened this issue 2 years ago • 23 comments

Where Storable

Description When running Storable::freeze on some structure, I got the following error, which is completely unhelpful and undocumented: "Unexpected object type (8) in store_hook()"

I think it is necessary, if Storable throws off exceptions like this, that they be properly documented so that we know what corrective action to take.

jackdeguest avatar Jul 18 '22 09:07 jackdeguest

Where Storable

Description When running Storable::freeze on some structure, I got the following error, which is completely unhelpful and undocumented: "Unexpected object type (8) in store_hook()"

I think it is necessary, if Storable throws off exceptions like this, that they be properly documented so that we know what corrective action to take.

Examination of the source code in dist/Storable/Storable.xs shows that this error message is a default (fallback) response generated only when one of about 7 other conditions is not met. Hence, the error message is accurate in the sense that the condition you encountered is truly unexpected.

Can you supply more information about what you were trying to do when you encountered this error?

jkeenan avatar Jul 18 '22 13:07 jkeenan

Where Storable Description When running Storable::freeze on some structure, I got the following error, which is completely unhelpful and undocumented: "Unexpected object type (8) in store_hook()" I think it is necessary, if Storable throws off exceptions like this, that they be properly documented so that we know what corrective action to take.

Examination of the source code in dist/Storable/Storable.xs shows that this error message is a default (fallback) response generated only when one of about 7 other conditions is not met. Hence, the error message is accurate in the sense that the condition you encountered is truly unexpected.

Can you supply more information about what you were trying to do when you encountered this error?

Additional information would be particularly helpful, as this error condition has been present since Storable was brought into the Perl 5 core distribution in August 2000 and has apparently never been previously reported as problematic.

jkeenan avatar Jul 18 '22 13:07 jkeenan

Examination of the source code in dist/Storable/Storable.xs shows that this error message is a default (fallback) response generated only when one of about 7 other conditions is not met. Hence, the error message is accurate in the sense that the condition you encountered is truly unexpected.

Can you supply more information about what you were trying to do when you encountered this error?

use v5.36;
my $ua = HTTP::Promise->new;
my $prom = $ua->get( $uri )->then(sub
{
    my $r = shift( @_ );
    say( "Got response '$r'" );
    return( $r );
})->catch(sub
{
    my $e = shift( @_ );
    say( "Got error: $e" );
})->wait;
# $prom->result calls Module::Generic::SharedMem::write which freezes the 
# HTTP::Promise::Response object and stores it into shared memory so it can 
# be returned to the main process.
my $res = $prom->result; # <-- This is where the exception gets raised
my $resp = $res->[0];
die( "$uri: ", $resp->status_line ) if( $resp->is_error );
my $data = $resp->decoded_content_utf8 || die( "No content found for url $uri\n" );
# More processing, but did not get there.

The exception stack trace is:

Unexpected object type (8) in store_hook() at /usr/local/lib/perl5/5.36.0/x86_64-linux-thread-multi/Storable.pm line 370.
	eval {...} called at /usr/local/lib/perl5/5.36.0/x86_64-linux-thread-multi/Storable.pm line 370
	Storable::_freeze(CODE(0x55b5c3e6bbb0), HASH(0x55b5cc9bdb68)) called at /usr/local/lib/perl5/5.36.0/x86_64-linux-thread-multi/Storable.pm line 349
	Storable::freeze(HASH(0x55b5cc9bdb68)) called at /usr/local/lib/perl5/site_perl/5.36.0/Module/Generic/SharedMem.pm line 777
	Module::Generic::SharedMem::__ANON__[/usr/local/lib/perl5/site_perl/5.36.0/Module/Generic/SharedMem.pm:780] called at /usr/local/lib/perl5/site_perl/5.36.0/Module/Generic/SharedMem.pm line 780
	eval {...} called at /usr/local/lib/perl5/site_perl/5.36.0/Module/Generic/SharedMem.pm line 780
	Module::Generic::SharedMem::write(Module::Generic::SharedMem=HASH(0x55b5cc092118), HASH(0x55b5cc9bdb68)) called at /usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm line 925
	Promise::Me::result(Promise::Me=HASH(0x55b5cc62fad0), HTTP::Promise::Response=HASH(0x55b5cca21c10)) called at /usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm line 868
	Promise::Me::__ANON__[/usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm:890] called at /usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm line 890
	eval {...} called at /usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm line 890
	Promise::Me::resolve(Promise::Me=HASH(0x55b5cc62fad0), HTTP::Promise::Response=HASH(0x55b5cca21c10)) called at /usr/local/lib/perl5/site_perl/5.36.0/curry.pm line 19
	curry::__ANON__[/usr/local/lib/perl5/site_perl/5.36.0/curry.pm:20](HTTP::Promise::Response=HASH(0x55b5cca21c10)) called at /usr/local/lib/perl5/site_perl/5.36.0/HTTP/Promise.pm line 174
	HTTP::Promise::__ANON__[/usr/local/lib/perl5/site_perl/5.36.0/HTTP/Promise.pm:175](URI::https=SCALAR(0x55b5cc4c14e8)) called at /usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm line 631
	Promise::Me::__ANON__[/usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm:667] called at /usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm line 667
	eval {...} called at /usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm line 667
	Promise::Me::exec(Promise::Me=HASH(0x55b5cc62fad0)) called at /usr/local/lib/perl5/site_perl/5.36.0/Promise/Me.pm line 1088
	Promise::Me::wait(Promise::Me=HASH(0x55b5cc62fad0)) called at ./scripts/check_uri.pl line 329

jackdeguest avatar Jul 18 '22 21:07 jackdeguest

use v5.36;
my $ua = HTTP::Promise->new;
my $prom = $ua->get( $uri )->then(sub

I don't see a declaration of, or assignment to, $uri.

jkeenan avatar Jul 18 '22 21:07 jkeenan

I don't see a declaration of, or assignment to, $uri.

Do not worry about this. $uri is declared. It just does not show in this extract.

I could track down the issue to a glob object.

use Module::Generic::File::IO;
my $io = Module::Generic::File::IO->new( __FILE__, 'r' );
my $serialised = Storable::freeze( $io );
Unexpected object type (8) in store_hook() at /usr/local/lib/perl5/5.36.0/x86_64-linux-thread-multi/Storable.pm line 370.
	eval {...} called at /usr/local/lib/perl5/5.36.0/x86_64-linux-thread-multi/Storable.pm line 370
	Storable::_freeze(CODE(0x561108546b78), Module::Generic::File::IO=GLOB(0x56110bd72e60)) called at /usr/local/lib/perl5/5.36.0/x86_64-linux-thread-multi/Storable.pm line 349
	Storable::freeze(Module::Generic::File::IO=GLOB(0x56110bd72e60)) called at ./t/17.serialise.t line 93

I modified Module::Generic::File::IO to implement a STORABLE_freeze subroutine, but it does not even get called, so I do not get any chance to make this right.

Also, setting $Storable::forgive_me to a true value as pointed out in the Storable doc provides no relief. The documentation states: "The store functions will croak if they run into such references unless you set $Storable::forgive_me to some TRUE value. In that case, the fatal message is converted to a warning and some meaningless string is stored instead.", but this does not appear to be true since I got an exception.

jackdeguest avatar Jul 18 '22 22:07 jackdeguest

Do not worry about this. $uri is declared. It just does not show in this extract.

Giving an incomplete extract is not very helpful when people try to reproduce your problem.

Leont avatar Jul 18 '22 23:07 Leont

Giving an incomplete extract is not very helpful when people try to reproduce your problem.

Sure. I will be more careful.

jackdeguest avatar Jul 18 '22 23:07 jackdeguest

I could track down the issue to a glob object.

Can you reduce it to a complete but actually minimal example?

Leont avatar Jul 19 '22 00:07 Leont

object type 8 is SVt_REGEXP, but regexps should be supported.

Leont avatar Jul 19 '22 00:07 Leont

Can you reduce it to a complete but actually minimal example?

Yes. I thought I did in the revised snippet posted

jackdeguest avatar Jul 19 '22 00:07 jackdeguest

Your reduced case, with some debugging code added:

$ cat gh-19964-xyz.pl
use 5.36.0;
use Storable;
use Data::Dump qw( dd pp );
use Module::Generic::File::IO;
my $io = Module::Generic::File::IO->new( __FILE__, 'r' );
say "AAA";
dd $io;
{
    local $@;
    my $serialised;
    eval { $serialised = Storable::freeze( $io ); };
    if (length $@) { say "BBB: $@"; } else { say "BBB: no dollar at"; }
}
say "Finished!";

Run with perl-5.36.0.

$ THISPERL gh-19964-xyz.pl 2>&1 
AAA
do {
  require Symbol;
  my $a = bless(Symbol::gensym(), "Module::Generic::File::IO");
  *{$a} = {
    _exception_class => "Module::Generic::Exception",
    _init_params_order => [],
    _init_strict_use_sub => 1,
    _log_handler => "",
    _message_default_level => 0,
    _msg_no_exec_sub => 0,
    colour_close => ">",
    colour_open => "<",
    debug => 0,
    error_max_length => "",
    level => 0,
    verbose => 0,
    version => "v0.1.0",
  };
  $a;
}
BBB: Can't store GLOB items at /home/jkeenan/testing/v5.36.0/lib/perl5/5.36.0/amd64-freebsd-thread-multi/Storable.pm line 370, at gh-19964-xyz.pl line 11.

Finished!

jkeenan avatar Jul 19 '22 00:07 jkeenan

Note: for the previous comment I had to force-install HTTP::Promise because one of its indirect prerequisites is CryptX, which is experiencing test failures not yet resolved upstream; see https://github.com/DCIT/perl-CryptX/issues.

jkeenan avatar Jul 19 '22 00:07 jkeenan

BBB: Can't store GLOB items at /home/jkeenan/testing/v5.36.0/lib/perl5/5.36.0/amd64-freebsd-thread-multi/Storable.pm line 370, at gh-19964-xyz.pl line 11.

Can this be managed, though? One does not always have direct control over a possible glob buried in a module.

The Storable documentation states that "The store functions will croak if they run into such references unless you set $Storable::forgive_me to some TRUE value. In that case, the fatal message is converted to a warning and some meaningless string is stored instead."[1], but this does not appear to be working.

[1] https://metacpan.org/pod/Storable#BUGS

jackdeguest avatar Jul 19 '22 01:07 jackdeguest

BBB: Can't store GLOB items at /home/jkeenan/testing/v5.36.0/lib/perl5/5.36.0/amd64-freebsd-thread-multi/Storable.pm line 370, at gh-19964-xyz.pl line 11.

This is a different and expected error.

Unexpected object type (8) in store_hook()

The type number here isn't an SvTYPE() result, but an internal type number from Storable's sv_type(), the 8 is svis_OTHER.

This error occurs when the SV is blessed into class with a STORABLE_freeze(), but then store_hook() doesn't recognize the object type, which appears to be a GLOB in this case.

I can get the same error with:

tony@venus:.../git/perl5$ ./perl -Ilib -MStorable=freeze -le 'freeze(*STDERR{IO}); sub IO::File::STORABLE_freeze {}'
Unexpected object type (8) in store_hook() at lib/Storable.pm line 370, at -e line 1.

or more similarly (since *STDERR{IO} is an IO not a GLOB):

tony@venus:.../git/perl5$ ./perl -Ilib -MStorable=freeze -le 'freeze(IO::File->new("/dev/null")); sub IO::File::STORABLE_freeze {}'
Unexpected object type (8) in store_hook() at lib/Storable.pm line 370, at -e line 1.

This also happens with regexps, since I didn't consider STORABLE_freeze when I added regexp support:

tony@venus:.../git/perl5$ ./perl -Ilib -MStorable=freeze -le 'freeze(qr/quux/); sub Regexp::STORABLE_freeze {}'
Unexpected object type (7) in store_hook() at lib/Storable.pm line 370, at -e line 1.

tonycoz avatar Jul 19 '22 01:07 tonycoz

I'm working on fixes for this, the new error message is:

tony@venus:.../git/perl5$ ./perl -Ilib -MStorable=freeze -le 'freeze(IO::File->new("/dev/null")); sub IO::Handle::STORABLE_freeze {}'
Unexpected object type (GLOB) of class 'IO::File' in store_hook() calling IO::Handle::STORABLE_freeze at lib/Storable.pm line 370, at -e line 1.

which is a bit wordy, but all of the added information is useful.

I want to add support for regexps too, but I'm not sure it's possible.

tonycoz avatar Jul 19 '22 11:07 tonycoz

I think it would be useful and fair play to check if the package has a subroutine STORABLE_thaw to give it a chance to return a proper glob object. Currently, Storable creates a SCALAR object, if $Storable::forgive_me is set to a true value, so it would be nice to give an opportunity to the package to return an object based on the deserialised data. Allowing the subroutine STORABLE_freeze, if any, to return a value to Storable would be good too. Currently, doing so creates this type (8) error.

jackdeguest avatar Jul 19 '22 12:07 jackdeguest

That would make globs a very special case for GLOBs.

If we make the return value of STORABLE_thaw[1] significant for existing types, any existing implementations that happen to return a ref, so handling GLOBs would be confusingly special.

It might be possible to do this with the STORABLE_attach hook instead, but as documented that's intended for connecting to system shared objects rather than objects that belong to the object being frozen.

I'll look further into it.

[1] I'm not fond of the pre-creation mechanism for STORABLE_thaw, in particular it assumes that the if the implementation of a class is currently a HASH (or whatever) it won't be changed to another implementation. This will especially be a problem if Corinna needs its own SV type.

tonycoz avatar Jul 21 '22 05:07 tonycoz

I do not understand all the intricacies of the inner working of Storable, so please bear with my ignorance, or naive comment. Right now, for glob type objects, to avoid Storable from crashing, as per the documentation, one can set $Storable::forgive_me to a true value and by doing so Storable would by itself arbitrarily create a SCALAR-based object, notwithstanding whether this is suitable or not for the operation of the module. In other words, My::Package=GLOB would rather be something like My::Package=SCALAR. Given this, and if the package has implemented expressly a STORABLE_thaw subroutine, why not let it return whatever suitable object? Isn't it proper to recognise the module itself knows better what to return to make it work? If one looks at how Sereal or CBOR works with FREEZE and THAW subroutines, it seems to me this approach would work nicely.

jackdeguest avatar Jul 21 '22 05:07 jackdeguest

I could change the code that calls STORABLE_thaw to use the return value instead of passing in an object, but it would break every existing implementation of STORABLE_thaw that expects the current protocol.

The alternative would be a new hook (STORABLE_thaw_returns?) that returns the object instead of being supplied an SV to modify, but I'm hesitant to add another thaw-like hook when we already have two.

tonycoz avatar Jul 21 '22 05:07 tonycoz

I could change the code that calls STORABLE_thaw to use the return value instead of passing in an object

I am not sure I understand. This does not align with what I experienced with glob-based modules, in that Storable does not even call a STORABLE_thaw even if it exists. I checked when debugging, and it did not get called. Instead, Storable thaw would create a dummy object based on a scalar.

At any rate, I am not saying the current api should change for other types of module. The way it works is fine for other types. I am only referring to glob-based modules.

jackdeguest avatar Jul 21 '22 05:07 jackdeguest

Here is what I did to make it work. I changed the subroutine Storable::thaw to the following that I called instead:

sub thaw {
    my ($frozen, $flags) = @_;
    $flags = $Storable::flags unless defined $flags;
    return undef unless defined $frozen;
    my $self;
    my $da = $@;			        # Could be from exception handler
    eval { $self = Storable::mretrieve($frozen, $flags) };# Call C routine
    if ($@) {
        $@ =~ s/\.?\n$/,/ unless ref $@;
        logcroak $@;
    }
    $@ = $da;
    # Here is what I added ↓
    if( Scalar::Util::reftype( $self ) eq 'SCALAR' && 
        $$self =~ /^You[[:blank:]\h]+lost[[:blank:]\h]+GLOB\([^\)]+\)/ &&
        $self->can( 'THAW' ) )
    {
        return( $self->THAW );
    }
    return $self;
}

Is there something wrong in doing this?

jackdeguest avatar Jul 21 '22 05:07 jackdeguest

The problem with using the return value of STORABLE_thaw

I am not sure I understand. This does not align with what I experienced with glob-based modules, in that Storable does not even call a STORABLE_thaw even if it exists. I checked when debugging, and it did not get called. Instead, Storable thaw would create a dummy object based on a scalar.

I was suggesting a change, such a change would also involve modifying STORABLE_freeze.

At any rate, I am not saying the current api should change for other types of module. The way it works is fine for other types. I am only referring to glob-based modules.

Storable doesn't know or care about the module, only the SV types of the values it receives. A given package could use a mixture of hash, array and scalar based objects, though it would be unlikely, and probably better done with distinct packages.

My problem with giving STORABLE_thaw distinct protocols depending on whether the object is GLOB based or non-GLOB based is documenting 'we have this interface, except this'.

This can be solved by defining a STORABLE_freeze for the class that contains a reference to the GLOB based object.

The lack of support for GLOBs isn't new.

tonycoz avatar Jul 25 '22 01:07 tonycoz

I think this issue can be solved with a more comprehensive, non-breaking approach that would consist of having two special hooks: STORABLE_freeze_pre_process and STORABLE_thaw_post_process and ensuring Storable accepts and uses instead the output of STORABLE_freeze_pre_process for freezing, and Storable::thaw calls and returns the output of STORABLE_thaw_post_process if it exists.

Doing so, would give a fighting chance to a glob-based module to return a suitable non-glob-based object back to Storable before it gets frozen.

Likewise, the post process hook would give a fighting chance to a XS based module to ensure the deserialised object is actually a usable object.

Implementing those 2 hooks would not break anything and would give more power and flexibility to each module who knows better than Storable how to prepare their object.

This is exactly what I have just implemented in my workaround with Storable::Improved and using all of Storable test units plus some others for glob and XS module, it works well.

jackdeguest avatar Jul 25 '22 02:07 jackdeguest