I have the following procedure that refreshes access and refresh tokens from Xero accounting.
If I uncomment the jObject.Free
line, then the xeroResponse.Free
line causes an Access Violation.
I have looked at other code doing similar things, and some free the TJSONObject
and TRESTClient
, TRESTRequest
and TRESTResponse
objects. And some don't.
The code below works, but I'm not sure if I have memory leaks because I'm not freeing something I should be. And, is there an order I need to free things in? Or, does the below procedure have flaws and should be written differently?
And, do TJSONValue
and TJSONArray
need to be freed or not?
procedure CheckXeroTokenValidity;
var
xeroClient: TRESTClient;
xeroRequest: TRESTRequest;
xeroResponse: TRESTResponse;
jObject : TJSONObject;
sRefreshToken : string;
begin
//this procedure will check to see if the access token is older than 30 minutes and will refresh it, if the refresh token if not more than 60 days old
with dmProcs do begin
ResetTable(tXero, '', True);
tXero.First;
if not tXero.Eof then begin
//check to see if the access token is expired or almost expired - if it is, refresh it if the refresh token is less than 60 days old
if MinutesBetween(tXeroDateToken.AsDateTime, Now) > 25 then begin
//check to see if the refresh token is still valid
if DaysBetween(tXeroDateToken.AsDateTime, Date) < 59 then begin
//get the current refresh token
sRefreshToken := tXeroRefreshToken.AsString;
xeroClient := TRESTClient.Create(nil);
xeroRequest := TRESTRequest.Create(nil);
xeroResponse := TRESTResponse.Create(nil);
Screen.Cursor := crHourGlass;
try
xeroClient.ContentType := 'application/json';
xeroResponse.ContentType := 'application/json';
xeroRequest.Client := xeroClient;
xeroRequest.Response := xeroResponse;
xeroRequest.Method := TRESTRequestMethod.rmPOST;
xeroRequest.Params.Clear;
xeroRequest.Params.AddItem('grant_type', 'refresh_token');
xeroRequest.Params.AddItem('client_id', sClientID);
xeroRequest.Params.AddItem('refresh_token', sRefreshToken);
xeroClient.BaseURL := 'https://identity.xero.com/connect/token';
try
xeroRequest.Execute;
if Assigned(xeroResponse.JSONValue) then begin
//parse the JSON
jObject := TJSONObject.Create;
jObject := xeroResponse.JSONValue as TJSONObject;
if (xeroResponse.StatusCode = 200) and (Assigned(jObject.GetValue('access_token'))) then begin
try
tXero.Edit;
tXeroAccessToken.AsString := jObject.GetValue('access_token').Value;
tXeroRefreshToken.AsString := jObject.GetValue('refresh_token').Value;
tXeroDateToken.AsDateTime := Now;
tXero.Post;
except on E:Exception do
ShowMessage('Error: ' + E.Message);
end;
end else begin
//if an error occured or a new access and refresh token was not supplied, then delete the xero record
tXero.First;
if not tXero.Eof then begin
tXero.Delete;
end;
if Assigned(jObject.GetValue('error')) then begin
ShowMessage('Error: ' + jObject.GetValue('error').ToString);
end;
end;
end;
except on E:Exception do
ShowMessage('Error: ' + xeroResponse.ErrorMessage + ' - ' + E.Message);
end;
finally
Screen.Cursor := crDefault;
//jObject.Free;
xeroClient.Free;
xeroRequest.Free;
xeroResponse.Free;
end;
end else begin
Information('Please see your manager to re-establish your accounting connection.');
end;
end;
end;
end;
end;
If I uncomment the
jObject.Free
line, then thexeroResponse.Free
line causes an Access Violation.
You are freeing a TJOSNObject
that TRESTResponse
owns, and tries to free after you have already freed it.
The code below works, but I'm not sure if I have memory leaks because I'm not freeing something I should be.
Yes, you do have a memory leak. You are creating your own TJSONObject
, and then discarding (leaking) it to use the TJSONObject
that TRESTResponse
provides to you. Get rid of the TJSONObject.Create
statement entirely, you don't need it.
And, do
TJSONValue
andTJSONArray
need to be freed or not?
In this situation, no.
TRESTResponse.JSONValue
is owned by TRESTResponse
, so you do not need to worry about freeing it. And that TJSONObject
owns its sub-values, so you don't need to worry about freeing them, either.
The only objects that need to be freed in this code are the REST objects you are creating yourself. And even then, I would suggest you make them own each other, so you only have 1 object to free yourself, eg:
xeroClient := TRESTClient.Create(nil);
xeroRequest := TRESTRequest.Create(xeroClient);
xeroResponse := TRESTResponse.Create(xeroClient);
...
xeroClient.Free;
FYI, on a side note:
//if an error occured or a new access and refresh token was not supplied, then delete the xero record tXero.First; if not tXero.Eof then begin tXero.Delete; end;
You are already on the 1st record at this point in the code, so there is no need to call tXero.First()
or check tXero.Eof
here. You are processing only the 1st record in the DataSet, you are not looping through the DataSet, but even if you were, going back to the 1st record to delete it would be wrong. You should be deleting only the active record that you are processing.