Briefly. About “partially initialized objects” (or again about factories)

Original in Russian: http://programmingmindstream.blogspot.ru/2014/10/blog-post_88.html

Based on:

Briefly. About constructors
Depression, or Falsity of hResult and other ErrorCode
Briefly. Once again about factories (in Russian)
Briefly. Again about factories
Briefly. About factories
Briefly. “Why we need tests” (in Russian)
Factory method (in Russian)

Today, after all, (I hope) I found an error that didn’t “let me live” many years.

We could trace it in various logs.

But we could not “vasten”  it.

But today I “seized it by the tail”, under a debugger.

Although it sounds trite – the reason is the same old “partially initialized objects”.

Something like this (naturally, I simplified the code to the maximum):

construstor TStorageHeader.Create(aPosition: Int64);
begin
 Assert(aPosition >= 0);
 inherited Create;
 FillChar(f_Data, SizeOf(f_Data), 0); // - we initialize the data
 f_Position := aPosition;
 LockFile(f_Position, SizeOf(f_Data)); // - we lock the file region where EXCEPTION can be RAISED - LOCK_VIOLATION
 LoadData(f_Position, f_Data, SizeOf(f_Data)); // - we read data 
end;
 
destructor TStorageHeader.Destroy;
begin
 SaveData(f_Position, f_Data, SizeOf(f_Data)); // - we write data 
 UnLockFile(f_Position, SizeOf(f_Data)); // - we unlock the file region
 inherited;
end;

What is the problem here?

This one is:

var
 l_H : TStorageHeader;
begin
 l_H := TStorageHeader.Create(aSomePosition);
 try
  ...
 finally
  FreeAndNil(l_H);
 end;//try..finally
end;

- if there’s NO EXCEPTION in the constructor, all goes well.

But if there IS an EXCEPTION in the constructor, the destructor is called AUTOMATICALLY.

On the PARTIALLY INITIALIZED object.

What happens? If exception is triggered in LockFile, then LoadData code will fail.

But! The destructor will be called (we read documentation).

I quote:

"Destructors called because of exceptions thrown from constructors"

So, what happens?

SaveData code will save what FillChar has initialized instead of what we've READ.

Do you understand the problem?

It is trivial. However, it was “alive” for many years. Why? Because the probability of the exception raised from LockFile was extremely low. Due to the fact that locking is adjusted to time-out, which is adequate for the most operations to eliminate the exception.

Moreover – we had code (above) that synchronised the access to the resources at a level of “business logic”.

But!

The synchronization caused a negative effect on the performance. The “bottleneck”  (in Russian).

Customers “waited for each other” and kept out of the “shared resource”. That is why we did not come to LockFile that might “trigger the exception”. It’s just – “all waited in line” and worked in series instead of in parallel.

(By the way, it is worth reading - E.W. Dijkstra “Co-operating sequential processes”  (in Russian). Although it has “nothing to do with the case”.)

But!

We managed this “bottleneck” and customers started to work “truly independently and in parallel”.

The probability of getting the exception and the “problem code” – has increased dramatically.

Especially on automatic tests  (in Russian) , since they work without delays and pauses of human being.

As a result, the error began to appear more regularly and more often.

Especially it appears when we launch tests under a debugger and pause at break-points “in the right place” emulating “network latencies”.

What should we do?

Well, it’s simple. For example, we can do the following:

construstor TStorageHeader.Create(aPosition: Int64);
begin
 Assert(aPosition >= 0);
 inherited Create;
 f_DataIsLoaded := false; // - for “safety”... in reality, to POSTULATE our intentions
 FillChar(f_Data, SizeOf(f_Data), 0); // - we initialize data
 f_Position := aPosition;
 LockFile(f_Position, SizeOf(f_Data)); // - we lock the file region where EXCEPTION can be RAISED - LOCK_VIOLATION
 LoadData(f_Position, f_Data, SizeOf(f_Data)); // - we read data
 f_DataIsLoaded := true; // - we postulate that we have read data
end;
 
destructor TStorageHeader.Destroy;
begin
 if f_DataIsLoaded then // - we only write what we have READ, not “garbage”
  SaveData(f_Position, f_Data, SizeOf(f_Data)); // - we write data 
 UnLockFile(f_Position, SizeOf(f_Data)); // - we unlock the file region
 inherited;
end;

Or in this way:

construstor TStorageHeader.Create(aPosition: Int64);
begin
 Assert(aPosition >= 0);
 inherited Create;
 FillChar(f_Data, SizeOf(f_Data), 0); // - we initialize data
 f_Position := aPosition;
 LockFile(f_Position, SizeOf(f_Data)); // - we lock the file region where EXCEPTION can be RAISED - LOCK_VIOLATION
 // - we also “raise the flag” - IsLocked
 LoadData(f_Position, f_Data, SizeOf(f_Data)); // - we read data 
end;
 
destructor TStorageHeader.Destroy;
begin
 if Self.IsLocked then // - we write only if we “lock the region", hence – we’ve read it
  SaveData(f_Position, f_Data, SizeOf(f_Data)); // - we write data 
 UnLockFile(f_Position, SizeOf(f_Data)); // - we unlock the file region
 inherited;
end;

But “for my taste” – it is better to make "factory method"

Something like this:

construstor TStorageHeader.InternalCreate(aPosition: Int64);
begin
 inherited Create;
 FillChar(f_Data, SizeOf(f_Data), 0); // - we initialize data
 f_Position := aPosition;
 LoadData(f_Position, f_Data, SizeOf(TData)); // - we read data 
end;
 
class function TStorageHeader.Create(aPosition: Int64): TStorageHeader;
var
 l_H :TStorageHeader; 
begin
 Assert(aPosition >= 0); // - all PRECONDITIONS are transferred from CONSTRUCTOR to FACTORY METHOD
 // Next – we transfer ALL code that POTENTIALLY raises exceptions from constructor to FACTORY METHOD:
 LockManager.LockFile(aPosition, SizeOf(TData)); // - we lock the file region where EXCEPTION can be RAISED - LOCK_VIOLATION
 // - if the exception is raised, it does not come to the constructor (hence – destructor)
 //   and we do not have to raise “flags”
 l_H := InternalCreate(aPosition);
 Result := l_H;
end;
 
destructor TStorageHeader.Destroy;
begin
 SaveData(f_Position, f_Data, SizeOf(f_Data)); // - we write data 
 UnLockFile(f_Position, SizeOf(f_Data)); // - we unlock the file region
 inherited;
end;

I also recommend to read this - BeforeRelease  (in Russian).

The article gives reasons why “saving code” has to be executed before destructing the object.

As for me, it is obvious that the “undestructed object” has to be saved.

It is worth to move all “saving code” from destructor to BeforeRelease (BeforeDestruction (in Russian)).

https://groups.google.com/forum/#!topic/borland.public.delphi.language.objectpascal/d7ajc3wkngY

I quote:

"I just stumbled upon some behaviour of Delphi, which I find pretty
confusing: I prefer using "AfterConstruction" and "BeforeDestruction"
for creating/destroying class members. Now, if an exception is raised in
"AfterConstrucion", Delphi immediately destroys the object (ok so far)
but "BeforeDestruction" is not called (not ok). Online help is not clear
about this, but one could guess that it is *always* called. My
conclusion is: always override "Destroy" instead of "BeforeDestruction"
to safely clean up. Is that right? What's the use of "BeforeDestruction"
then?"

In fact - is ok.

We read documentation:

http://docwiki.embarcadero.com/VCL/2010/en/System.TObject.BeforeDestruction

I quote:

"BeforeDestruction is called automatically before the object's first destructor executes. Do not call it explicitly in your applications."

"Note:  BeforeDestruction is not called when the object is destroyed before it is fully constructed. That is, if the object's constructor raises an exception, the destructor is called to dispose of the object, but BeforeDestruction is not called."

May be it is written in a “florid style” and the problem is made “out of the thin air” (I suppose), but I will be glad if you “rethink” it and find such errors (not simple, though trivial) of your own.

If you do not have them, I am all the more glad for you :-)

I do not want to make anybody "happy by force"  (in Russian) . But what if…

May be, I’ve got “butter fingers” and other people do not have such problems, but it is hardly so.

P.S. One more thing (Briefly. “Why we need tests”  (in Russian)) – the error “lived” for 15 years already. It “poisoned existence”, but did not “prevent from living”.

During this time hundreds (even thousands) of users (internal ones) worked with it.

But!

One should have made tests (Briefly. Load tests have been made  (in Russian) ) – and the error “crept out” at once. Despite the fact that it was found not on “hundreds of clients”, but on two. I repeat, on two… Two but DETERMINED ones.

So, begin to think... About the “extrapolation”  ...

On two...

I’ll just say - “When you tell “I have no time for this”, in fact you mean it is not important for you. No time for self-development? For sport, for family?”  (in Russian).

And also - “Nobody controls a developer, but himself who “slaps on the wrist”. This is not masochism. Instead a developer gets something important - the feel of stability and confidence in his professional future. He develops two times more than usually. He creates, but he does not “plough”. He actually ploughs less  (in Russian).