It is safe to change variable values of a thread from the main thread?

Question

I wrote a simple component that monitors a folder and triggers an event when it detects changes. It works well... apparently. But I'm not sure of one thing. From time to time, the main thread may need to update the monitored path and I'm not sure if I've done this right. It is about the SetNewPath procedure. This is executed from the main thread and it changes the UpdatePath variable from the other thread. It is possible to create an conflict when the main thread writes to UpdatePath and the component thread tries to read its value in the Execute cycle ?

FolderMonitor.pas

unit FolderMonitor;

interface

uses
  SysUtils, Windows, Classes, ExtCtrls;

type
  TOnFolderChange = procedure(Sender: TObject) of object;

  TFolderMonitor = class(TThread)
  private
    MainWait: THandle;
    UpdatePath: Boolean;
    TimeOut: Cardinal;
    FPath: String;
    FOnFolderChange: TOnFolderChange;
    procedure DoOnFolderChange;
    procedure SetNewPath(Path:String);
  protected
    procedure Execute; override;
  public
    constructor Create(const FolderPath: String; OnFolderChangeHandler: TOnFolderChange);
    destructor  Destroy; override;
    procedure   Unblock;
    property    Path: String read FPath write SetNewPath;
    property    OnFolderChange: TOnFolderChange read FOnFolderChange write FOnFolderChange;
  end;

implementation

constructor TFolderMonitor.Create(const FolderPath: String; OnFolderChangeHandler: TOnFolderChange);
begin
  inherited Create(True);
  FOnFolderChange:=OnFolderChangeHandler;
  FPath:=FolderPath;
  UpdatePath:=false;
  FreeOnTerminate:=false;
  MainWait:=CreateEvent(nil,true,false,nil);
  Resume;
end;

destructor TFolderMonitor.Destroy;
begin
  CloseHandle(MainWait);
  inherited;
end;

procedure TFolderMonitor.DoOnFolderChange;
begin
  if Assigned(FOnFolderChange) then
  Synchronize(procedure
  begin
   FOnFolderChange(Self);
  end);
end;

procedure TFolderMonitor.Unblock;
begin
  PulseEvent(MainWait);
end;

procedure TFolderMonitor.SetNewPath(Path:String);
begin
  FPath:=Path;
  UpdatePath:=true;
  PulseEvent(MainWait);
end;

procedure TFolderMonitor.Execute;
var Filter,WaitResult: Cardinal;
    WaitHandles: array[0..1] of THandle;
begin
  Filter:=FILE_NOTIFY_CHANGE_DIR_NAME + FILE_NOTIFY_CHANGE_FILE_NAME + FILE_NOTIFY_CHANGE_SIZE;
  WaitHandles[0]:=MainWait;
  WaitHandles[1]:=FindFirstChangeNotification(PWideChar(FPath),false,Filter);
  TimeOut:=INFINITE;

  while not Terminated do begin
   if UpdatePath then begin
    if WaitHandles[1]<>INVALID_HANDLE_VALUE then FindCloseChangeNotification(WaitHandles[1]);
    WaitHandles[1]:=FindFirstChangeNotification(PWideChar(FPath),false,Filter);
    TimeOut:=INFINITE;
    UpdatePath:=false;
   end;

   if WaitHandles[1] = INVALID_HANDLE_VALUE
    then WaitResult:=WaitForSingleObject(WaitHandles[0],INFINITE)
    else WaitResult:=WaitForMultipleObjects(2,@WaitHandles,false,TimeOut);

   case WaitResult of
    WAIT_OBJECT_0: Continue;
    WAIT_OBJECT_0+1: TimeOut:=200;
    WAIT_TIMEOUT: begin DoOnFolderChange; TimeOut:=INFINITE; end;
   end;

   if WaitHandles[1] <> INVALID_HANDLE_VALUE then
    FindNextChangeNotification(WaitHandles[1]);
  end;

  if WaitHandles[1] <> INVALID_HANDLE_VALUE then
   FindCloseChangeNotification(WaitHandles[1]);
end;

end.

UnitMain.pas

unit UnitMain;

interface

uses
  Windows, Messages, SysUtils, Variants, Classes, Graphics, Controls, Forms,
  Dialogs, StdCtrls, FolderMonitor;

type
  TForm1 = class(TForm)
    Memo1: TMemo;
    Edit1: TEdit;
    Button1: TButton;
    procedure FormCreate(Sender: TObject);
    procedure OnFolderChange(Sender: TObject);
    procedure FormDestroy(Sender: TObject);
    procedure Button1Click(Sender: TObject);
  end;

var
  Form1: TForm1;
  Mon: TFolderMonitor;
  X: integer;

implementation

{$R *.dfm}

procedure TForm1.FormCreate(Sender: TObject);
begin
 X:=0;
 Mon:=TFolderMonitor.Create('D:\Test',OnFolderChange);
end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
 Mon.Terminate;
 Mon.Unblock;
 Mon.WaitFor;
 Mon.Free;
end;

procedure TForm1.OnFolderChange(Sender: TObject);
begin
 inc(x);
 Memo1.Lines.Add('changed! '+IntToStr(x));
end;

procedure TForm1.Button1Click(Sender: TObject);
begin
 Mon.Path:=Edit1.Text;
end;

end.

Yes, it is unsafe to let one thread modify a string field whilst another thread can read it at the same time, — TLama, May 23 '15 at 11:35

score 0 · Accepted Answer · edited May 23 '17 at 11:59

0

You have a variable shared between multiple threads, with one thread modifying the variable. This scenario is known as a data race.

Some races may be benign. This one is not. If one thread modifies the variable whilst another thread reads it, errors may occur. Because the data type is complex (pointer to heap allocated array of characters) it is quite possible for the reading thread to attempt to read from deallocated memory.

For a complex type like this you need to use a mutual exclusion lock whenever you access the value. All reads and writes must be serialised by the lock. Use a critical section or a monitor.

To ensure that you don't ever perform unprotected access it is wise to enforce this rule in code. For example, my TThreadSafe<T> described here: Generic Threadsafe Property

edited May 23 '17 at 11:59

Community

1
1

answered May 24 '15 at 08:20

David Heffernan

601,492
42
1,072
1,490

No. Serialization of a block (or blocks) of code means that only one thread can be executed that block at any one time. Forcing all access to occur in the same thread is a way to achieve serialization. However, it is a very inefficient serialization method. Using a mutual exclusion lock allows you to let code execute on different threads, and there is only a delay if two or more threads are in contention for that lock. – David Heffernan May 24 '15 at 09:53
Generally speaking, forcing execution to happen on a specific thread is usually performed only when that code has an affinity to a specific thread. The classic example is UI code which has affinity to the UI thread. But to ensure threadsafe shared access of a variable, mutual exclusion is generally the right solution. For certain data types, atomic operations are available (known as lock-free synchronization) that can perform better than a full blown mutual exclusion lock. But that's not viable for a complex type like string. – David Heffernan May 24 '15 at 09:56
Thank you for the explanations and I apologize you because I deleted my comment. I was reading the TCriticalSection help and found there that all threads must use critical sections to ensure the safety. – Marus Gradinaru May 24 '15 at 10:17
I think you are wrong. String assignment is implemented with XCHG, which makes it thread-safe. Other thread may read old value (if XCHG was not performed yet) or new value (if XCHG has finished), but it is not possible for other thread to read garbage, trash or "partially updated" value. – Alex May 24 '15 at 22:53
@alex No. I am right. You have it wrong. Thread A reads the pointer. Thread B then destroys that pointer. Thread A then dereferences the now invalid pointer. – David Heffernan May 25 '15 at 05:05
@DavidHeffernan, Thread B can not destroy the pointer, because it is a string, which have reference count > 0 (since it is hold & used in Thread A). – Alex May 25 '15 at 05:07
@Alex Not so. Threads don't hold references. Variables do. Both threads access through the same variable. I'm afraid you've got this badly wrong. – David Heffernan May 25 '15 at 05:11
@DavidHeffernan, threads can hold references by using variables ;) In order for thread to access "invalid pointer" f, it has to use some variable for that. It has to be variable. And since it is a string variable - it has a reference counter. Which means, as long as you hold it in your variable - its reference counter is +1. For example, the code in question uses that variable to make assignment. Therefore, it increases lock counter on string. Which means other thread can not free that string. – Alex May 25 '15 at 05:18
@Alex I'm afraid you are deeply confused over this. I can't really begin to put you right in comments. Not only do you not understand string thread safety, but you don't understand reference counting. I'm not going to argue with you. It seems pointless. Your mind is set. – David Heffernan May 25 '15 at 05:27
All you need is the exact scenario for bad things to happen. – Alex May 25 '15 at 05:48

It is safe to change variable values of a thread from the main thread?

1 Answers1