0

I want to check the following code, in particular Execute method,to ensure it's not doing anything super dodgy. This code is Delphi 2007, btw.

I am passing back to the main thread via synchronize a reference to an object (FToken) created in the execute method of the thread.

I am only doing this after the FToken is created in TTokenThread.Execute

Is this OK in terms of thread safety?

Critique most welcome.

Type

  TExceptionNotifyEvent = procedure(Const Error : String) of object;
  TTokenNotifyEvent = procedure(const Token : AccessToken) of object;

  TTokenThread = class(TThread)
  private
     FTokenNotifyEvent : TTokenNotifyEvent;
     FOnException : TExceptionNotifyEvent;
     FExceptionMsg : String;
     FToken : AccessToken;
     procedure HandleException(const E : Exception);
     procedure DoException;
     procedure DoSynchronise;
  public
     constructor Create;
     destructor Destroy;override;
     procedure Execute; override;
     procedure WhenException(const OnException : TExceptionNotifyEvent);
     procedure WhenToken(const TokenNotify : TTokenNotifyEvent);
  end;




{ TTokenThread }
constructor TTokenThread.Create;
begin
  inherited create(true);
  FreeOnTerminate := true;
end;

destructor TTokenThread.Destroy;
begin
  if assigned(FToken) then FToken.free;
  inherited;
end;

procedure TTokenThread.Execute;
begin
  inherited;
  try
    FToken := RESTAdapter.GetAccessToken();
    synchronize(DoSynchronise);
  except on
    E:Exception do
    begin
      handleException(E);
    end;
  end;
end;

procedure TTokenThread.HandleException(const E: Exception);
begin
  if terminated then exit;
  FExceptionMsg := E.Message;
  synchronize(DoException); //synch call with the main thread (this is potentially blocking)
  Terminate;
end;

procedure TTokenThread.WhenException(const OnException: TExceptionNotifyEvent);
begin
  FOnException := OnException;
end;

procedure TTokenThread.WhenToken(const TokenNotify: TTokenNotifyEvent);
begin
  FTokenNotifyEvent := TokenNotify;
end;

procedure TTokenThread.DoException ;
begin
  if assigned(FOnException) then FOnException(FExceptionMsg);
end;


procedure TTokenThread.DoSynchronise;
begin
  if assigned(FTokenNotifyEvent) and assigned(FToken) then FTokenNotifyEvent(FToken);
end;

calling code


procedure TForm3.ProcessToken(const Token : AccessToken);
begin
  if assigned(Token) then
  begin
      memo1.lines.clear;
      memo1.lines.add(format('access_token: %s',[Token.Token]));
      memo1.lines.add(format('expires_in: %s',[inttostr(Token.ExpiryUtc)]));
      memo1.lines.add(format('token_type: %s',[token.type2]));
      memo1.lines.add(format('scope: %s',[token.scope]));
  end;
end;



procedure TForm3.BtnAccessTokenClick(Sender: TObject);
var
  TokenThread : TTokenThread;
begin
  TokenThread := TTokenThread.create;
  TokenThread.WhenToken(ProcessToken);
  TokenThread.Resume;
end;


  • 1
    Your use of `Synchronize()` is fine. However, you did not show where `RESTAdapter` is being created, or specify whether `GetAccessToken()` is thread-safe, or where `WhenException()` and `WhenToken()` are being called. So, whether the rest of your code is safe is unknown. – Remy Lebeau Apr 19 '23 at 04:10
  • Thanks. RESTAdapter.GetAccessToken() is a class function. – The Pudding Race Apr 19 '23 at 04:30
  • TTokenThread.WhenException is not right. Naming convention here a bit poor as well. – The Pudding Race Apr 19 '23 at 04:39
  • 1
    "*RESTAdapter.GetAccessToken() is a class function*" - that doesn't address the issue of whether or not it is thread-safe. – Remy Lebeau Apr 19 '23 at 04:56
  • yeah, I know :) The reason I don't know is because that is calling crosstalk....I'm assuming that it's OK to call crosstalk via a thread. But I have no idea. Thanks for bringing that to my attention. For anyone not familar with crosstalk it is calling a C# library behind the curtains – The Pudding Race Apr 19 '23 at 04:58
  • That would be a question to ask AToZed directly, or the CrossTalk community. https://www.atozed.com/crosstalk/support/ – Remy Lebeau Apr 19 '23 at 05:09
  • 1
    The FToken processing is ok IMHO, but I'd rather not have the `WhenException` and `WhenToken` procedures. I would pass event handler procedures to the constructor of the TTokenThread, because it is how the handlers (or data) are supposed to be assigned to TThread. It is not safe to change them when the thread is running. For example code: `if assigned(FTokenNotifyEvent) and assigned(FToken) then FTokenNotifyEvent(FToken);` - you in theory allow FTokenNotifyEvent to be changed to nil after checking assigned(FTokenNotifyEvent) but before FTokenNotifyEvent(FToken). – Julius Tuskenis Apr 19 '23 at 06:18
  • Good point Julias. Will update that. – The Pudding Race Apr 19 '23 at 06:22

0 Answers0