Why is my global variable "inaccessible" when debugging?

Question

I'm building an application that contains around 30 Forms. I need to manage sessions, so I would like to have a global LoggedInUser variable accessible from all forms. I read "David Heffernan"'s post about global variables, and how to avoid them but I thought it would be easier to have a global User variable rather than 30 forms having their own User variable.

So I have a unit: GlobalVars

unit GlobalVars;

interface
uses User; // I defined my TUser class in a unit called User
var
   LoggedInUser: TUser;

implementation

initialization
   LoggedInUser:= TUser.Create;
finalization
   LoggedInUser.Free;
end.

Then in my LoginForm's LoginBtnClick procedure I do :

unit FormLogin;

interface

uses
  [...],User;

type
  TForm1 = class(TForm)
   [...]
    procedure LoginBtnClick(Sender: TObject);
  private
    { Déclarations privées }
  public
  end;

var
  Form1: TForm1;
  AureliusConnection : IDBConnection;

implementation

{$R *.fmx}
uses [...]GlobalVars;

procedure TForm1.LoginBtnClick(Sender: TObject);
var
  Manager : TObjectManager;
  MyCriteria: TCriteria<TUser>;
  u : TUser;
begin
  Manager := TObjectManageR.Create(AureliusConnection);
   MyCriteria :=Manager.Find<TUtilisateur>
    .Add(TExpression.Eq('login',LoginEdit.Text))
    .Add(TExpression.Eq('password',PasswordEdit.Text));
  u := MyCriteria.UniqueResult;
  if u = nil then
   MessageDlg('Login ou mot de passe incorrect',TMsgDlgType.mtError,[TMsgDlgBtn.mbOK],0)
  else begin
   LoggedInUser:=u;     //Here I assign my local User data to my global User variable
   Form1.Destroy;
   A00Form.visible:=true;
  end;
  Manager.Free;
end;

Then in another form I would like to access this LoggedInUser object in the Menu1BtnClick procedure :

Unit C01_Deviations;

interface

uses
  System.SysUtils, System.Types, System.UITypes, System.Classes, System.Variants, 
  FMX.Types, FMX.Graphics, FMX.Controls, FMX.Forms, FMX.Dialogs, FMX.StdCtrls,
  FMX.ListView.Types, FMX.ListView, FMX.Objects, FMX.Layouts, FMX.Edit, FMX.Ani;

type
  TC01Form = class(TForm)
   [...]
Menu1Btn: TButton;
[...]
procedure Menu1BtnClick(Sender: TObject);

  private
    { Déclarations privées }
  public
    { Déclarations publiques }
  end;

var
  C01Form: TC01Form;

implementation
uses [...]User,GlobalVars;
{$R *.fmx}

procedure TC01Form.Menu1BtnClick(Sender: TObject);
var
  Assoc : TUtilisateur_FonctionManagement;
  ValidationOK : Boolean;
  util : TUser;
begin
  ValidationOK := False;
  util := GlobalVars.LoggedInUser; // Here i created a local user variable for debug purposes as I thought it would permit me to see the user data. But i get "Inaccessible Value" as its value
  util.Nom:='test';
  for Assoc in util.FonctionManagement do  // Here is were my initial " access violation" error occurs
  begin
    if Assoc.FonctionManagement.Libelle = 'Reponsable équipe HACCP' then
    begin
      ValidationOK := True;
      break;
    end;
  end;
  [...]
end;

When I debug I see "Inaccessible Value" in the value column of my user. Do you have any idea why ?

I tried to put an integer in this GlobalVar unit, and i was able to set its value from my login form and read it from my other form..

I guess I could store the user's id, which is an integer, and then retrieve the user from the database using its id. But it seems really unefficient.

Answer 1

"I assign this data to my global user" - what does this mean?

Through my crystal ball I see code similar to this in your Login form:

var
  user: TUser;
begin
  user := TUser.Create;
  try
    // assign properties/fields of user instance

    LoggedInUser := user;
  finally
    user.Free;
  end;
end;

after that, LoggedInUser points at deallocated (and possibly reused) block of memory. Addressing any property/field of LoggedInUser will likely result in access violation.

Ha, quite close:

LoggedInUser:=u;

Add a method Assign(aSource: TUser) to your TUser class, which does a deep copy of values for all fields/properties (not reference assignments) and call it instead:

LoggedInUser.Assign(u);

Answer 2

To perhaps add a bit more explanation, consider this section of code :

begin
  Manager := TObjectManageR.Create(AureliusConnection);
   MyCriteria :=Manager.Find<TUtilisateur>
    .Add(TExpression.Eq('login',LoginEdit.Text))
    .Add(TExpression.Eq('password',PasswordEdit.Text));
  u := MyCriteria.UniqueResult;

Now, TUser is a reference type so the variable u contains a pointer to the object. In this case u is now pointing to the UniqueResult object belonging to your TObjectManager instance.

  if u = nil then
   MessageDlg('Login ou mot de passe incorrect',TMsgDlgType.mtError,[TMsgDlgBtn.mbOK],0)
  else begin
   LoggedInUser:=u;     

//Here I assign my local User data to my global User variable

No. Here you are also making LoggedInUser point to the same object instance that u is pointing to, which is still the UniqueResult of your TObjectManager. In addition, since your initialization section in GlobalVars has already created an instance of TUser (that LoggedInUser was pointing to), you have just overwritten that reference and leaked memory.

   Form1.Destroy;
   A00Form.visible:=true;
  end;
  Manager.Free;
end;

And now you have freed your TObjectManager, destroying its UniqueResult - the object that both u and LoggedInUser were pointing to.

Igor's suggestion to perform a deep copy will fix both of these problems.

 LoggedInUser.Assign(u);

First, you will not leak memory since you will now be assigning values to your previously constructed TUser object (and not overwriting the only reference you have to it!). Second, when your TObjectManager is freed you will not destroy the object your LoggedInUser is referencing.

If TUser is a custom class you will need to implement your own method of making a deep copy. See here for an example. You don't need to derive from TPersistent, nor do you need to use Assign, necessarily, but you do need to provide some type of deep copy function in your TUser class.

Alternatively, from the documentation, have a look at the

OwnsObjects property

If true (default), all managed objects are destroyed when the TObjectManager object is destroyed. If false, the objects remain in memory.

So before you free your TObjectManager, simply do :

Manager.OwnsObjects := false;
Manager.Free;

And be careful to free any other objects returned from queries, etc, that you no longer need. This will allow you to assign the reference as you are doing now and it will prevent the TObjectManager from freeing it when it itself is destroyed.

Note that this solution still leaves you with your memory leak problem, so it should be dealt with by either checking for an instance and freeing it before making the new assignment or not constructing an empty TUser to begin with.

Answer 3

since you're creating an object instance in the global variable, that suggests you want to assign values to it later on, rather than assigning an entirely new object.

You can do that as suggested using the .Assign method, or you can copy the relevant fields one by one if there are just a few.

You DO want a "deep-copy" of the TUser object's fields, and also ensure that if you have any objects inside of TUser that you determine if they need to be deep-copied or if references can be copied instead.

But this is a common mistake people make. You want one or the other -- either create an instance and copy VALUES into it; or don't create the instance and assign another instance to it, like the one you get from TObjectManager. But as was also pointed out, the life-time of the one returned by TObjectManager might not be what you expect. (If it uses an Interface, it's automatically reference-counted and safe to assign to another object. But it may not be obvious without digging around whether that's the case or not.)

Since the TObjectManager is returning either an object reference or NIL, you could simply assign the TObjectManager's value directly to the global variable. In that case, don't create a default instance in the initialization section.

I also echo the sentiments around passing the object into the contstructors of the forms rather than using a global variable. This lets you set up unit testing.

I'd also like to add a comment that nobody else has touched upon yet.

  else begin
   LoggedInUser:=u;     //Here I assign my local User data to my global User variable
   Form1.Destroy;
   A00Form.visible:=true;
  end;

This is NOT how you destroy a form! Because, technically speaking, nothing after the call to Form1.Destroy is operating inside of a valid instance. It'll probably work ok most of the time because the stack probably hasn't become corrupted; but it's anybody's guess whether the A00Form.visible := true will work or not.

AND ... that's not how you close a form and set focus on another one anyway. This form should not know about any other forms. Again, that's something that should be injected when the form is created if it's needed, which it really isn't in this case.

Generally speaking, use the OnClose handler to do stuff you want to happen when a form closes. But in this case, you don't even want that.

You want to instantiate the form from elsewhere, then use ShowModal to display it. When it closes, you want to set up some kind of return status that says whether the person logged-in correctly or not. If they did, THEN open whatever the next form is you want them to see. If not, you probably want to show them the login form again with a message displayed on it.

This also hints at how you inject the TUser record into each form -- by wrapping the form opens inside of a method that creates a form instance, passing in the TUser object (whether via constructor or a property), then dispenses with the form after the ShowModal returns. This may mean you want to only HIDE the form on Close rather than FREE it, allowing the controlling Display method to access form data before destroying the form! (I believe that caHide is the default Action on the OnClose handler, so you'd only need to add an OnClose if you want to replace it with caFree. That's because the default behavior of the IDE is to auto-create forms, in which case you don't want them to free themselves when they close. If you don't auto-create your forms, then you DEFINITELY need to free them after creating them and displaying them with Show or ShowModal -- unless you choose to leave them hanging around in memory, which defeats the purpose of bypassing the auto-create in the first place.)