Is assigning undef required for DESTROY to run?

[MidLifeXis]

I have a vendor's library, distributed in binary mode only. It seems that if I create an object using this library, I need to assign undef to get it to call the DESTROY method. If I assign a new invocation of the item to the object variable, it core dumps.

This core dumps...

my $obj = Vendor::Library->new(...);
... code here ...
$obj = Vendor::Library->new(...);

This does not...

my $obj = Vendor::Library->new(...);
... code here ...
$obj = undef;
$obj = Vendor::Library->new(...);

I am probably assuming too much out of this vendor, but shouldn't the two snippets of code be expected to do the same thing, or are my expectations out of whack?

I will be submitting the bug in any case (I should not be able to force the library to core dump, IMO), but am evaluating my expectations.

------

After further research, I think I may have an explanation. In Conway's OOP, on page 430, it states:

A destructor is a method with the special name of DESTROY. It is called automatically when an object's reference count reaches zero.

In the first instance, the creation of the second object happens before the first object's refcount reaches 0, as the assignment happens after the new call. In the second instance, the assignment of undef causes the refcount to drop to 0, then the creation of the new object is possible to happen, then the assignment happens.

I know that I answered this myself. Documenting this here, with more information, may help another monk in the future.

[Corion]

It may be that your vendor library isn't too clean with its memory management. The order of object creation and object destruction differs whether you undef the first object manually or wait until it gets overwritten:

sub new{
    print 'new';
    bless{}
};

sub DESTROY{
    print'DESTROY'
};

$x=new;
$x=new;
__END__
new
new
DESTROY
DESTROY

and with explicit undef:

sub new{
    print'new';
    bless{}
};
sub DESTROY{print'DESTROY'};
$x=new;
undef $x;
$x=new
__END__
new
DESTROY
new
DESTROY

at least under Perl 5.10, but I don't see how it could be different under any Perl ≤ 5 due to the fact that the assignment will always happen after the object construction, and only after the assignment, the memory allocated for the first instance can be released.

Update: Doh - had I read your node to the end, I'd have seen that you came to that conclusion already...

[kennethk]

I want to know what happens with the code:

my $obj1 = Vendor::Library->new(...);
my $obj2 = Vendor::Library->new(...);

It sounds like they've got some funny internals that can't handle having more than one object instantiated at a time, in which case they should be checking for that in their new method.

[MidLifeXis]

Agreed, and that is what my bug report says :-). The connection is supposed to be one-at-a-time, which, to me as well, means that they should be catching if a second call is made before the first connection is released.

[Bloodnok]

More than that, IMO, the module shouldn't be advertising the constructor as an integral part of the interface - rather, something more like that shown in Monadic Classes.

[Tanktalus]

A few random thoughts that maybe the purveyor of your library could take note of:

• The new function typically is used for creating new objects. It makes little-to-no sense (to me) for a singleton. Better would be a package method of "get_instance" or something.
• Exposing your singleton object is required in straight-jacket-type languages. Not so much in Perl. Why not just Vendor::Library->some_method(), which would use code like this?

    my $_global;
    sub _self_or_create
    {
      my $self = shift;
      return $self if (ref $self and $self->isa(__PACKAGE__))
      $_global ||= __PACKAGE__->_new();
      return $_global; # assume $self eq __PACKAGE__ or something derive
  +d from it?
    }
  

  You could put more bondage into that by checking ref $self eq __PACKAGE__ instead of isa, but that's the basic idea. From there, the top of each function would simply look like:

    sub some_method
    {
      my $self = _self_or_create(shift);
      # ...
    }
  

  At this point, you're back to where you started, but the user doesn't need to instantiate anything, and then won't be tempted to do silly things like create two of them.
• And for you, maybe not having two creation statements in the same scope? :-)

  {
    my $obj = Vendor::Library->new(...);
    # code here
  }
  {
    my $obj = Vendor::Library->new(...);
    # different code here (I presume)
  }
  

  I have a (probably irrational) fear about explicitly assigning undef. It just smells funny to me. :-)

[MidLifeXis]

Re: code smell - yeah, there is enough code smell to go around. My code has grown up over 9 years now, and parts of it are in serious need of refactoring.

So many tuits, so little time.

[CountZero]

So it is some kind of a singleton object then? But badly written and with crappy documentation?

[MidLifeXis]

Yes it is a singleton, yes it is badly written, but no, it says it is a singleton. My code was abusing it. My issue with it is that it didn't protect itself (it stops me from doing everything else with it ;-)

Update: Just to be clear, I don't mean that it should protect me (necessarily) from my doing stupid stuff, but protect itself from being able to dump core. Dumping core implies memory corruption implies potential data corruption implies lack of trust in the underlying data storage.