Safely altering the behaviour of heavily used modules

[adamk]

I've just started using Storable heavily for the first time, to implement server-abstracted storable SQL queries. By heavily I mean I need to play a lot of games with STORABLE_freeze/thaw hooks to let some fairly complex internal structures store correctly.

Now unfortunately STORABLE_thaw, rather than just giving you the class name to create an object of, "helpfully" creates an empty (and of course invalid) "object", which it expects you to fill and correct so that it becomes a valid object.

It also runs in void context, and returns via the passed argument, which isn't very Perlish at all.

Unfortunately, this "helpfulness" makes STORABLE_thaw completely useless for things like Singletons or classes with pools of objects, since you can't correct the link to refer to the Singleton (you are just left with an empty, invalid and broken object).

There are a number of bugs in rt.cpan.org relating to this problem, and I've taken it on myself to resolve the problem, as it is hurting me badly. I've defined the new behaviour I think STORABLE_thaw should have as

1. STORABLE_thaw called in scalar rather than void context

2. If and only if STORABLE_thaw returns a _different_ object from the empty one passed in, that also isa() in the same package, use THAT object in the thawed version instead.

This change is functionally back-compatible, but will this break anything? Using File::Find::Rule::PPI to find every STORABLE_thaw subroutine in a minicpan checkout, I found that 2/3rds of them do either "return self;" or "return;" for safety sake and will thus act sanely, and the rest don't appear to be able to race easily (I've filed rt.cpan.org bugs against them detailing the one line change just in case).

After that long intro, my question is... "Is there anything else I can do to ensure a clean changeover to the new behaviour, assuming I can convince the Storable maintainers to accept the patch."

Update
See http://rt.cpan.org/NoAuth/Bug.html?id=4901 for ongoing progress of the proposal to change STORABLE_thaw

[brian_d_foy]

I was talking about this sort of thing with some New York Perl Mongers this evening (especially the part about the possibility that nobody accepts the patch).

I like to create subclasses that re-implement the parts that I don't like. It's a bit of work, but if, for some reason, someone decides to update Storable, the subclass is still there. I'm always really nervous about messing with other people's original source, unless I get some indication that they are going to accept the patch.

Good luck!

[kvale]

You have probably done this already, but it would be useful to add tests for both the new behavior situation and the old default. Check objects returned for both cases, which is something that may not be done in the current test suite. Check that freezing and thawing close the loop in both cases. I'm not sure what race easily means, but I am sure that you will have to deal with it in your proposal.

Add documentation describing changes and give example code where the new behavior may be useful.

[adamk]

Well, by "race easily" I mean for example that some uses of STORABLE_thaw are something like...

sub STORABLE_thaw {
    my ($self, $string, @refs) = @_;

    ...

    %$self = %rebuilt_contents;
}

Now, if called in scalar context, that should return the number of elements in the listified hash, which won't be a problem. But it's only safe accidentally, not by design. The original author was told that anything that might be returned would be ignored.

But there is the potential for one of these cases where the function doesn't have an explicit "return;" to accidentally return a different object of the same class, which the new behaviour would interpret as an alternate object and treat incorrectly.

[hv]

I think you're going about this the wrong way: this is not a backwards-compatible change, and since Storable is a very heavily used module there are bound to be many STORABLE_thaw methods out there that are not on CPAN, and that will be broken by this change.

The only way you could make a backward compatible change to the way STORABLE_thaw is called would be to change something that would otherwise be guaranteed to be an error, and I think that is likely to be difficult to arrange.

Alternative approaches: add a new method that Storable could look for as well as/in preference to STORABLE_thaw; add a completely new interface to Storable; or subclass Storable.

Hugo

[adamk]

Hmm... well perhaps I should have said it is "intended" to be backwards-compatible. And in the strictest sense, if every use of STORABLE_thaw did a "return;" to return nothing it would be.

That said, you are right on the possible ways of doing it provably safely.

But things get ugly quickly... STORABLE_thaw2? STORABLE_freezify/thawify? Storable::SaneThawing?

None of these are particularly elegant, and you could argue that many just make the problem of bad interfaces worse.

Unless of course, we moved to Storable2 and redesigned the API. Although Storable works well enough, I must say I do find the plethora of similar-but-different functions somewhat PHP'esk.

[demerphq]

Seems to me that the problem with storable is that assignment to $_[0] doesn't work as it should work, or more specifically, as it would work in a normal perl subroutine.

For instance when I read this node my first though was "Hmm, you mean

sub STORABLE_thaw {
    $_[0]= $singleton;
}

doesn't work?". And of course when I tried it, it didn't.

Anyway, it seems to me that making assignment to @_ possible in the STORABLE_freeze() hook would resolve your problem, and I think would be backwards compatible.

Ill just add the mandatory shameless plug: with Data::Dump::Streamer this is easy.

package Foo;
use Data::Dump::Streamer;

my $singleton=bless {},'Foo';
my $proxy=bless [],'Bar';
my $nope;

sub DDS_freeze {
    return if $nope;
    return ($proxy,"->DDS_thaw")
}

sub Bar::DDS_thaw {
    $singleton;
}

my @array=($singleton,$singleton,$singleton);

my $array=Dump(\@array)->Names('array')->Out();
print "$array\n---\n";
eval $array or die $@;
$nope=1;

Dump($singleton,\@array,$array)->Names(qw(singleton *array array))->Ou
+t();
__END__
$array = [
           bless( [], 'Bar' )->DDS_thaw(),
           'V: $array->[0]',
           'V: $array->[0]'
         ];
$array->[1] = $array->[0];
$array->[2] = $array->[0];

---
$singleton = bless( {}, 'Foo' );
@array = (
           $singleton,
           $singleton,
           $singleton
         );
$array = [
           $singleton,
           $singleton,
           $singleton
         ];

[adamk]

After a huge amount of debate and discussions (see that link to RT) and some money slipped the way of an XS expert, this problem was solved, and solved both fully back-compatibly and in the best possible manner.

See "STORABLE_attach" in the Storable POD for details.

[demerphq]

Regardless as to whether you added a new method. IMO Assignment to @_ should work as expected.