FreeAndNil or NilAndFree ?

[Guest]

Shouldn't current FreeAndNil named right ? Should right naming matter ?

And the critical question is : which is safer ?
free-then-nil (if any exception raised in an object destructor then the links/chains/pointers will still hold the needed of information to track)
nil-then-free (when exception raises the pointer is already lost, minimizing the chance to understanding how did this happen)

Shouldn't such exception let be to cause full collapse of the application instead of covering one incident and hope will not escalate, IMHO letting it crash the application successfully would be better and safer, so the bug (the cause) can be found and fixed right,

What do you think ?

[David Heffernan 2345]

Destructors must not raise exceptions. My personal view is that if they do then the best course of action is to terminate the process.

[sakura 45]

Further on, the function is to ensure a properly nulled reference, even if something fails. A object that failed during Free/Destroy is usually not usable any longer anyways.

[Mahdi Safsafi 225]

Quote

free-then-nil (if any exception raised in an object destructor then the links/chains/pointers will still hold the needed of information to track)

I really don't understand your point here. But if I'm understanding it correctly, you want to track other fields ? if so, there is no guarantee that you're going to have a valid information(Once a destructor is called ... you can no longer rely on the object).

 

[Guest]

7 minutes ago, Mahdi Safsafi said:

 there is no guarantee that you're going to have a valid information

It would when mechanism of logging is in place, i don't want the failed object itself only but with its parent, also memory dump like Windows memory crash dump will give a hand to figure it out, but not when it is already nil, the stack will point to the calling code, that is OK, but the field value itself if it is in an array is lost or when it is not a field to this parent, and tracking all of that can be much easier if the value is still there.

[Mahdi Safsafi 225]

51 minutes ago, Kas Ob. said:

It would when mechanism of logging is in place

No it wouldn't on a multi-thread app ! The time the instance A is destroyed, its free-memory could be used immediately by another thread that creates a new object B. Your reference then is pointing to the new object B instead of A. This applies to all the things that are allocated on the heap. 

[Guest]

6 minutes ago, Mahdi Safsafi said:

No it wouldn't on a multi-thread app

I am talking about different thing, in almost in all of my application i have a logging mechanism that log the creation of many critical objects (it can log everything) but for default it log the critical things and most the time will keep that part in memory without flushing it down to disk until an exception been raised or wrong behaviour observed, so what i meant by logging i can compare the value in that pointer before it removed with that log to see what object it was with how and when it been created in the first place, and i know if the logging is there then i am logging the destruction too, but it is not the point here, and i can log everything everywhere in production.

 

as for multi thread application then yes nilling it might help in recovering from exception in the destructor but it would be better to find prevent/fix the exception in the first place. 

 

Most important thing, accessing in a safe multithreaded way should not be allowed until at one thread is finished, so if a thread is destroying it, then it should not matter if it was free-then-nil or nil-then-free while the execution in still in protected section, right ?

[Stefan Glienke 2002]

FreeAndNil is correct from the consumer point of view - you call the routine it frees the thing and then once it returns you have a nil reference.

That it sets the reference to nil before it actually calls Free is an implementation detail to protect against potentially recursive attempts to FreeAndNil the same reference or accessing an object which is undergoing destruction and probably a result of the convoluted architecture of TComponent and Co.

Edited July 14, 2020 by Stefan Glienke

[Mahdi Safsafi 225]

1 hour ago, Kas Ob. said:

I am talking about different thing.

Possibly as I said before, I may misunderstanding you. My point was that you shouldn't rely on the object after a call to free was made regardless whether its destructor raised exception or not because the time the RTL frees the object A, another thread can reclaim to create a new object B and possibly it would occupy the same address space as the recent destroyed one A. Later, trying to read object A.SomeField could possibly crash with B.SomeField.

 

Quote

Most important thing, accessing in a safe multithreaded way should not be allowed until at one thread is finished, so if a thread is destroying it, then it should not matter if it was free-then-nil or nil-then-free while the execution in still in protected section, right ?

As long as there was no implicit/explicit call to allocate memory by any thread (including your current thread) that happened after freeing your object and before accessing to that dead object.

i.e:

procedure DoSomething(Obj: TMyObject);
var Data: Pointer;
begin
  try
    FreeThenNil(obj);
  except
    // reference is not null !
    // multithread:
    Log(Obj.FList); // buggy on MT as the instance could be already free and used by another object from another thread.
    // singlethread:
    Data := GetMemory(256); // if the instance was free, current call CAN use old instance data.
    InitData(Data);
    Dump(Data, Obj.Flist);  // buggy too ! FList may crach with Data.
  end;
end;

In a nutshell, as it was pointed out before, a destructor shouldn't raise exception and you shouldn't try to access an object after a call to free.

So nil-then-free makes more sense. 

 

EDIT:

Assume FList is not destroyed inside the destructor or assume its an integer type.

Edited July 14, 2020 by Mahdi Safsafi

[Guest]

@Mahdi Safsafi I agree with your explanation but is the object memory freed if and i am here talking about an imaginary raised exception in the destructor (which is wrong, yet it might happen), or the memory is still there allocated and will not be freed, now to the special case when nil is after free, here we have is better chance to get it by log or by memory leak detection, that was one, second when nil is after free, reusing it by different thread or just reusing will most likely raise an exception and point exactly how this happen and where it did come from, just saying these both cases can be argued to nil after free.

 

 

[Mahdi Safsafi 225]

27 minutes ago, Kas Ob. said:

@Mahdi Safsafi I agree with your explanation but is the object memory freed if and i am here talking about an imaginary raised exception in the destructor (which is wrong, yet it might happen), or the memory is still there allocated and will not be freed, now to the special case when nil is after free, here we have is better chance to get it by log or by memory leak detection, that was one, second when nil is after free, reusing it by different thread or just reusing will most likely raise an exception and point exactly how this happen and where it did come from, just saying these both cases can be argued to nil after free.

 

 

Yes it may happen that a destructor throws an exception i.e: AV. But usually you would spot them when debugging.

In a complex inheritance hierarchy its hard to tell whether the instance was freed or not after an exception. 

destructor TMyClass.Destroy();
var List: TList;
begin
  List := FList;
  inherited; // free instance.
  for i := 0 to 10 do
    List[i].Free(); // <--- AV here.
  List.free();
end;

As you can see, this class is freeing its instance before freeing its internal data. Some class can first free its internal data and finally free its instance.

So if you really need to access an object field inside an exception block, the safe way to do is to move first that field to the stack before calling the free(provided that Obj.destructor does not free that field):

procedure DoSomething(Obj: TMyObject);
var LList: TList;
begin
  LList := Obj.FList;
  try
    FreeThenNil(obj);
  except
    Log(LList);
  end;
end;

For me, I try to pay much more attention to my destructor code and I consider an exception thrown by a destructor as a fatal exception that requires terminating the app immediately because it may lead to a corrupted data (some fields were freed, some not), it may lead to never-ending leak, ... 

[David Heffernan 2345]

4 hours ago, Kas Ob. said:

might help in recovering from exception in the destructor

Nothing can save you at this point.

[aehimself 396]

2 hours ago, David Heffernan said:

Nothing can save you at this point.

Agreed. A destructor might be called even when the object creation is incomplete... so it must have proper "defenses" in place.

Let's just think on the standard way of local object handling...

myobject := TMyObject.Create;
Try
 myobject.DoStuff;
Finally
 myobject.Free;
End;

What happens if an exception is raised in the destructor? Uncaught exceptions and 99% of the times memory leaks.

4 hours ago, Kas Ob. said:

i am here talking about an imaginary raised exception in the destructor (which is wrong, yet it might happen)

No, it might not. If it does, it's a bug. If it's an external component, report it to the developer. If it is yours, fix it.

3 hours ago, Mahdi Safsafi said:

As you can see, this class is freeing its instance before freeing its internal data.

I might be fresh in the area, but what is the purpose of this? I would simply call it a bad code as up until now, I never really had the need to put inherited anywhere else than the very last line of a custom object destructor.

[Mahdi Safsafi 225]

49 minutes ago, aehimself said:

I might be fresh in the area, but what is the purpose of this? I would simply call it a bad code as up until now, I never really had the need to put inherited anywhere else than the very last line of a custom object destructor.

Not always a bad code ! sometime you need to make a lock/release. So you must call the inherited in between and finally free the Lock object. Sometime you need to perform a notification, ... Just take a look at the RTL/VCL, there're many destructor that call inherited in between.

So if you can control your destructor about how to call inherited ... there is no way to control other descendant-destructors that do not belong to you. i.e inheriting from a VCL/RTL class that does not call inherited as a last line in its destructor.

 

[David Heffernan 2345]

9 minutes ago, Mahdi Safsafi said:

Just take a look at the RTL/VCL

That something may be done in the RTL/VCL doesn't mean it is good practise.

[Mahdi Safsafi 225]

3 minutes ago, David Heffernan said:

That something may be done in the RTL/VCL doesn't mean it is good practise.

I didnt say its a good practice. All what I said there are some situations where its implemented. 

[David Heffernan 2345]

9 hours ago, Mahdi Safsafi said:

Not always a bad code ! sometime you need to make a lock/release. So you must call the inherited in between and finally free the Lock object. Sometime you need to perform a notification, ... Just take a look at the RTL/VCL, there're many destructor that call inherited in between.

 

9 hours ago, Mahdi Safsafi said:

I didnt say its a good practice. All what I said there are some situations where its implemented. 

I can't reconcile these two statements. 

[Mahdi Safsafi 225]

1 hour ago, David Heffernan said:

 

I can't reconcile these two statements. 

Quote

Not always a bad code

If I meant it to be a good practice, I'd say its not a bad code.

My point was that no one should rely on the object after a call has been made to free regardless whether an exception occurred or not ... and I still retain that position. For that point, I gave some example just for explanation but never considered it as a good practice. A good practice from my point of view is to expect the worse not to expect all people are following the good practice.

BTW, those destructors for the VCL/RTL (the way are implemented) are not safe.

Hope that this explicit statement clarify things for you.