Jump to content
Renate Schaaf

Why isn't this dangerous, or is it?

Recommended Posts

type
  TDoSomething = class
    Danger: string;
    constructor create;
    procedure DoSomething;
  end;

implementation
  ...
constructor TDoSomething.create;
begin
  Danger := 'Danger!';
end;

procedure TDoSomething.DoSomething;
begin
  ShowMessage('Doing something');
  Sleep(1000);
  ShowMessage('Done doing something');
  Free; //<-----
end;

procedure TForm2.Button1Click(Sender: TObject);
begin
  var
    DoSomething: TDoSomething := TDoSomething.create;
  DoSomething.DoSomething;
  // ShowMessage(DoSomething.Danger);
end;

initialization
ReportMemoryLeaksOnShutDown := true;
end.

I'm asking because I just noticed that I was doing something similar in my code :classic_blush:, only it was a bit more hidden. It has worked without complaint for quite a while, so I was wondering why. My guess is, it's because the 'Free' is the very last thing I ask the class to do. In the destructor the class is still alive, after that the memory is freed, and as long as I don't do anything like the commented ShowMessage, that should be safe or no?

I am cleaning this up, of course, but I didn't want to miss the chance of learning something, I don't understand what's going on in the Delphi-source at this point all that well.

 

Renate

Share this post


Link to post
27 minutes ago, Renate Schaaf said:

as long as I don't do anything like the commented ShowMessage, that should be safe or no?

After the "Free" the reference is a "dangling pointer". The ShowMessage could still work for centuries but only by luck as the memory where the class was is now free for other things.

So no, its not safe.

Try to use explicit Free's and you don't have to keep the implementation details in your head. Or keep it there and follow the rules.

Share this post


Link to post
30 minutes ago, Attila Kovacs said:

After the "Free" the reference is a "dangling pointer".

That's why I don't use it :). 

33 minutes ago, Attila Kovacs said:

Try to use explicit Free's and you don't have to keep the implementation details in your head.

Good advice.

 

34 minutes ago, Attila Kovacs said:

Or keep it there and follow the rules.

So you are saying it is Ok (other than maintainance headaches) to keep it there on condition that one follows the rules? I'm saying this, because I remember reactions like "Shriek" when somebody posted code like this.

Share this post


Link to post
3 minutes ago, Renate Schaaf said:

So you are saying it is Ok

Well, it was the last option 😉 As the example shows, this pattern hiding something from you. It's not your friend.

5 minutes ago, Renate Schaaf said:

other than maintainance headaches

Do not underestimate that. With time, you won't remember everything. I mean, a thing. You won't remember anything. 😉

 

 

  • Like 2
  • Haha 3

Share this post


Link to post

My frontal lobe Latency is one-two minutes L1 cache.  one to two weeks L2 cache every else is water under the bridge.   excepting a few grudges.    🙂   

  • Like 1

Share this post


Link to post
1 hour ago, Renate Schaaf said:

So you are saying it is Ok

No I don't think so. Follow the actual rules. When you create an instance, destroy it in the same context that you created it. 

  • Like 2

Share this post


Link to post
Posted (edited)

I don't think I've ever seen a procedure of a class free the instance of the class it's being called from. Everything in me is screaming DON'T DO IT!

 

This is very bad practice. The object code may still be in memory after the call to Free but it's no longer dedicated to that object and if, per chance, some other operation suddenly uses that available memory, you've got an access violation.

 

And to make an example that shows the problem of the dangling pointer @Attila Kovacspoint out:

procedure TForm1.Button1Click(Sender: TObject);
begin
  var
    DoSomething: TDoSomething := TDoSomething.create;

  DoSomething.DoSomething;

  if Assigned(DoSomething) then begin
    ShowMessage('DoSomething Freed but dangling reference still exists');
    DoSomething.DoSomething; // <--- ERROR!
  end;
end;

It's better to free the memory outside of the class itself in a try-finally block:

procedure TDoSomething.DoSomething;
begin
  ShowMessage('Doing something');
end;

procedure TForm1.Button2Click(Sender: TObject);
begin
  var
    DoSomething: TDoSomething;

  DoSomething := TDoSomething.create;
  try
    DoSomething.DoSomething;
  finally
    DoSomething.Free;
  end;
end;

This makes it obvious from the calling procedure (the button click event) that the object is freed which lessens the likelihood of using it again because it's right there in the procedure instead of hidden in the class itself.

Edited by corneliusdavid
  • Like 2

Share this post


Link to post
1 minute ago, David Heffernan said:

No I don't think so. Follow the actual rules.

I'm doing that, unless I'm too dim to notice. I just wanted to understand why it doesn't blow up in my face .. oh well.

Share this post


Link to post
11 minutes ago, Renate Schaaf said:

I'm doing that, unless I'm too dim to notice

Create/Free are the calls that allocated/deallocate the memory for an object, an instance of TDoSomething in this case. In your original code, the call to Create in Button1Click but the Free happens in a procedure of TDoSomething--that's where the difference in context is.

 

12 minutes ago, Renate Schaaf said:

I just wanted to understand why it doesn't blow up in my face

The only reason it didn't blow up is because you don't reference the object (the DoSomething var) after the call to Free. Look at my first example code above where I point out where an ERROR could occur--that's what COULD have happened to you.

Share this post


Link to post

That is why FastMM and probably other memory managers have diagnostic features such as setting freed memory to a certain pattern. If you enable that your ShowMessage will most likely blow up because Danger would be an invalid string pointer.

Share this post


Link to post
57 minutes ago, Stefan Glienke said:

setting freed memory to a certain pattern

Thanks, I didn't know that.

Otherwise my question (starting with why) has, as far as I am concerned, been answered by Attila, and I  wouldn't want to use anymore bandwidth for it.

Share this post


Link to post
On 3/22/2022 at 4:17 PM, corneliusdavid said:

 

It's better to free the memory outside of the class itself in a try-finally block:

 

Absolutely. The inclusion of Free in the instance method is essentially a side-effect of the method and constitutes a violation of the SRP. What if I want to call DoSomething twice?

 

Also, as a matter of consistency I personally always call the inherited constructor--even when descending from TObject directly. I have no guarantee that TObject.Create will always be an empty method.

Share this post


Link to post

In general, when I create a new class, a new feature or other, I always think about the possibility that the class is derived (inheritance, polymorphism) and therefore writing a code like the one indicated I would not write it even if it were possible without side effects.

 

Bye

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×