4

Is it safe to change VirtualTreeView data from the secondary thread ? And if yes, should I use critical sections (or even Synchronize method) ?

I'm afraid that when I'll be writing to the VT's data record from the other thread, main thread invokes its repaint meanwhile and this refresh will cause reading of the same record at one time. I would supplement I'm using only 2 threads in the application.

Something like ...

type
  PSomeRecord = ^TSomeRecord;
  TSomeRecord = record
    SomeString: string;
    SomeInteger: integer;
    SomeBoolean: boolean;
end;

...
var FCriticalSection: TRTLCriticalSection; // global for both classes
...

procedure TMyCreatedThread.WriteTheTreeData;
var CurrentData: PSomeRecord;
begin
  EnterCriticalSection(FCriticalSection); // I want to protect only the record

  CurrentData := MainForm.VST.GetNodeData(MainForm.VST.TopNode);

  with CurrentData^ do // I know, the ^ is not necessary but I like it :)
    begin
      SomeString := 'Is this safe ? What if VT will want this data too ?';
      SomeInteger := 777;
      SomeBoolean := True;
    end;

  LeaveCriticalSection(FCriticalSection);

  MainForm.VST.Invalidate;
end;

// at the same time in the main thread VT needs to get text from the same data
// is it safe to do it this way ?

procedure TMainForm.VST_GetText(Sender: TBaseVirtualTree;
  Node: PVirtualNode; Column: TColumnIndex; TextType: TVSTTextType;
  var CellText: string);
var CurrentData: PSomeRecord;
begin
  EnterCriticalSection(FCriticalSection); // I want to protect only the record

  CurrentData := VST.GetNodeData(VST.TopNode);

  with CurrentData^ do
    begin
      case Column of
        0: CellText := SomeString;
        1: CellText := IntToStr(SomeInteger);
        2: CellText := BoolToStr(SomeBoolean);
      end;
    end;

  LeaveCriticalSection(FCriticalSection);
end;

// I'm afraid the concurrent field reading may happen only here with the private VT fields
// FNodeDataSize, FRoot and FTotalInternalDataSize, since I have Node.Data locked by the
// critical sections in the VT events, some of those may be accessed when VT is refreshed
// somehow

function TBaseVirtualTree.GetNodeData(Node: PVirtualNode): Pointer;
begin
  Assert(FNodeDataSize > 0, 'NodeDataSize not initialized.');

  if (FNodeDataSize <= 0) or (Node = nil) or (Node = FRoot) then
    Result := nil
  else
    Result := PByte(@Node.Data) + FTotalInternalDataSize;
end;

Update

I've added the critical sections to the code, is it really unsafe to call GetNodeData from TMyCreatedThread class, even if this function only returns a pointer to the record ?

Thanks a lot

Regards

1 Answers1

6

No, especially the way you're doing it.

VST is a visual control. So is MainForm, which you're referring to directly from your thread. GUI controls are not thread-safe, and shouldn't be accessed directly from a thread. In addition, you're referring to the global variable 'MainForm' from the thread. This is absolutely NOT thread-safe.

If you need to access the data for the VST from both your main form and a separate thread, don't store it directly in VST.Node.Data. Keep it in an external list that you can surround with a critical section or some other thread-safe method, and access that external list in the thread (locking it first) or your main form in the VST events. See TLockList in the Delphi RTL for an example of a lockable list, and TMultipleReadExclusiveWrite for a sample synchronization class.

Ken White
  • 123,280
  • 14
  • 225
  • 444
  • Thanks, I see what you mean, but theoretically, what happens if I just enclose both methods into the critical section blocks as in my updated question ? I know GUI controls are not thread-safe, but the GetNodeData function only returns the pointer to node's data and node is again a pointer. And as I've read somewhere, Invalidate is thread safe. –  Apr 07 '11 at 20:41
  • It's *never* safe to refer to `MainForm` from a thread. And because `Node.Data` is a pointer doesn't make any difference; it's a pointer to a block of memory that you're trying to access from multiple threads unsafely. `MainForm` is a pointer as well; the compiler just hides that fact from you. It has nothing to do with `Invalidate`; it has to do with things being accessed in a way that can lead to instability or corruption of data or memory in a way that can cause hard to track down problems. Why do it wrong when you can do it right instead without a lot more work? – Ken White Apr 07 '11 at 20:53
  • > it's a pointer to a block of memory that you're trying to access from multiple threads unsafely. - that's what I can't see, why unsafely ? I'm accessing them in CriticalSection blocks, so they shouldn't conflict with reading of FNodeDataSize, FRoot and FTotalInternalDataSize what is used in GetNodeData or am I wrong ? I will follow your advice with the common list variable very likely, but I'm wondering why is dangerous to call VST.GetNodeData in TMyCreatedThread. –  Apr 07 '11 at 21:15
  • It may be safe (now), if your code never changes. But there are better ways to do it, and from your code I don't see any reason to do it differently. (Unless, that is, you've eliminated a lot of code for your question to conserve space.) I look at it like this: Sometimes there are lots of ways to do things; it's almost always better to do them using a published means, because that way you're fairly well protected against changes. If there's a choice of doing something two ways - eg., the Windows API or an undocumented DLL call), I'll use the published API without a darned good reason not to. – Ken White Apr 07 '11 at 21:48
  • Taking back. It is partially unsafe because of possible conflict with reading just those FNodeDataSize, FRoot ... variables. They are accessed even in some painting methods which can be triggered, so to say, whenever. –  Apr 07 '11 at 21:49
  • 2
    Reading those string fields is unsafe, too. If two threads can access the same string at the same time, then that string's reference count needs to be at least 2. If both threads are accessing the string through the same single *variable* (which they are in this case, because there's only one data record for any node), then the reference count will be just 1. When either thread accesses the thread and sees a reference count of 1, it will assume that it has exclusive access to the string. – Rob Kennedy Apr 08 '11 at 00:22
  • You're right, but VT accesses the data only in the handled events whose I can lock into the critical sections (as I have in the code above now). The problem with concurrent field reading may IMHO happens only for the internal VT fields such as FNodeDataSize, FRoot ... which are accessed in the GetNodeData function as well as in some inner painting methods. –  Apr 08 '11 at 08:42