Marco Tech Blog

May 23, 2008

When Self in Nil... You Know You are in Trouble

Nothing psychological here... only a Delphi programming error I bump into from time to time.

For the non-Delphi programmers out there, self is like this in curly brace languages (C++, C#, Java). It might happen, it you mess things, that yo are debugging a method and realize that self (the object the method is being applied to) is nil, meaning there is no object. Quite often this result in an access violation as you start doing something the data of the the non-existing object.

At times, an object is nil because the constructor that should have returned it failed with an exception (a very spacial case because the object destructor is automatically executed, at least in Delphi). Similarly, an object can be nil because the function that should have returned the reference to the object failed with an exception. This was my case. In a very complex piece of code (with 5 levels of nested try(finally/except blocks) I ended up with code like:

try
  obj := GetObjectFromPool (...);
  obj.DoThisAndThat;
finally
  obj.ReleaseObject;
end;

Now this is trivially wrong, but those statements are 70 lines of code apart, not really easy to spot. Also, in theory GetObjectFromPool should never fail, or at least this is far from common. Also, I cannot really move the GetObjectFromPool call outside of the finally block (again, the code is quite convoluted), so I had to resort to a far-from-clean:

finally
  if Assigned (obj)
    obj.ReleaseObject;

posted by marcocantu @ 6:57PM | 10 Comments [0 Pending]

10 Comments

When Self in Nil... You Know You are in Trouble

I think I'd rather do it this way, myself:

 obj := GetObjectFromPool;
 if Assigned(obj) then
   try
    obj.DoThisAndThat;
  finally
    obj.ReleaseObject;
  end;

Comment by Jay Faubion on May 23, 19:59

When Self in Nil... You Know You are in Trouble

Maybe I am missing something here, but my
understanding of try..finally blocks is that the
proper usage is:

obj := GetObjectFromPool();
try
  obj.DoWhatEver()
finally
  obj.Free;
end;

This ensures that obj is not nil, and relies on the
default constructor mechanism to free the object if an
exception is raised in the constructor which is highly
unlikely.  If such an exception occurs it is likely
due memory constraints and as such should be handled
by the Application exception handler.

Although your solution works, I have seen code added
to the top of the try block before obj is created,
that produces an exception which then yields an AV in
the finally block since obj does not exist.  That kind
of coding error is much more likely when creating the
object within the try block instead of outside as the
first line before the try.  It is of course, the kind
of mistake only a developer that doesn't understand
exception handling makes, but I have seen lots of it.

I have found using the above code structure and a
seperate try..finally for each resource has saved me
tons of time in tracking down obscure errors.  As such
fixing try..finally blocks and/or introducing them is
usually one of the first tasks I perform when starting
a new maintenance project.

Comment by Larry Hengen [larry@NOSPAMhengenconsulting.com] on May 23, 20:08

When Self in Nil... You Know You are in Trouble

I've actually seen self = nil a few times, especially
in multi-threaded code.  I have a couple methods that
start with 

if not Assigned(self) then Exit;

That prevents the method from executing if it is
already freed.

Comment by Jim McKeeth [http://www.DavinciUnltd.com] on May 23, 20:44

When Self in Nil... You Know You are in Trouble

This of course should be:

obj := GetObjectFromPool (...);
if assigned(obj) then try
  obj.DoThisAndThat;
finally obj.ReleaseObject; end;

I'm sure all three pending comments are containing 
similar code :)

Comment by gabr [http://thedelphigeek.com] on May 23, 21:51

When Self in Nil... You Know You are in Trouble

"far-from-clean" ?

Why? I've gotten into the habit of testing proper
assignment before trying to free stuff... :)

Comment by Fernando Madruga [http://memyselfanddelphi.blogspot.com] on May 23, 23:24

When Self in Nil... You Know You are in Trouble

I know the stardard way of coding an
allocate-try-use-finally-release block, thanks for
your suggestions. But this was within a very complex
piece of code, having many different paths of
execution. There are about 150 lines in the method,
with up to 5 nested try blocks. The lines shows in the
snippet are far apart, and there is exception handling
code that in most cases repairs the problem creating
another object.
As Jim points out, there are circumstances in which
you really don't know if you have a valid object or
not, so you test before every call or wihint the
method itself. not a standard practice, but precious
when needed.

Comment by Marco Cantù [http://www.marcocantu.com] on May 24, 01:26

When Self in Nil... You Know You are in Trouble

If you can't afford to refactor this monstrum, then 
I'd do one of those:

a) Either test the 'obj' immediately after it is 
retrieved from the pool and exit if it is not 
allocated (or throw exception, whichever more 
appropriate), or

b) Test if 'obj' is assigned each time before it is 
used.

Comment by gabr [http://thedelphigeek.com] on May 24, 08:59

When Self in Nil... You Know You are in Trouble

Which is precisely why Free is coded the way it is
(and why Free is non-virtual and why you [should]
never call Destroy [virtual] directly but always call
Free).

Same could easily be made to apply here.

ReleaseObject could/should be NIL safe in the same way
that Free is, and invoke a virtual Release mechanism
only if self is not NIL.

As for the snippet being "trivially" wrong, absolutely
not. It was BADLY wrong, plain and simple, no if's no
buts, no qualification.

Your "finally" ensured that some method was called on
an object for which no reference was assured.  You
imposed a guarantee on a non-guaranteeable outcome. 
The correct solution (without addressing the NIL
safety of your release mechanism) should be to address
the breach of contract between Get/Release and the
try/finally, not to apply a Band-Aid to the incorrect
(as coded) finally clause.

imho

Comment by Jolyon Smith [http://www.deltics.co.nz] on May 26, 00:15

When Self in Nil... You Know You are in Trouble

Another possible approach: Free checks for Self being
nil.  Could you code ReleaseObject to do the same
thing?  It is presumably a "free"-ish sort of function.

Comment by David M on May 26, 03:06

The snippit is not wrong . . . . maybe

We cannot assume the snippit is wrong because we
cannot know the value of obj before we entered the try
finally block.  If we initialize it to nil then the
block is completely legal.

obj := nil;
try
  obj := GetObjectFromPool (...);
  obj.DoThisAndThat;
finally
  obj.ReleaseObject;
end;

Assuming ReleaseObject is non-virtual and it checks
its own self reference internally.  Much in the same
way that Free works.

Comment by Jim McKeeth [http://www.DavinciUnltd.com] on June 10, 23:25

Post Your Comment

Click here for posting your feedback to this blog.

There are currently 0 pending (unapproved) messages.