View Issue Details

IDProjectCategoryView StatusLast Update
0037612LazarusLCLpublic2020-08-22 22:27
ReporterMartin Friebe Assigned ToMartin Friebe  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platform64bit IntelOSwin 10 
Product Version2.1 (SVN) 
Fixed in Version2.2 
Summary0037612: TLazThreadedQueue.Grow may loose items in the queue
DescriptionTLazThreadedQueue.Grow starts with FMonitor.Enter;

Therefore it is apparently meant to be used, once the queue is in action (i.e. the method is threadsave, threadsave access is expected once the threats will push and pop items.)

But, modifying the queue-size, without adjusting FTotalItemsPushed / FTotalItemsPopped means:
The position of the current item FList[FTotalItemsPopped mod FQueueSize] changes. So any further "pop" will get random data.

The following could fix this, but there are public properties to access those values. So user apps would no longer get the expected values.

procedure TLazThreadedQueue.Grow(ADelta: integer);
begin
  FMonitor.Enter;
  try
    FTotalItemsPushed := FTotalItemsPushed mod FQueueSize;
    FTotalItemsPopped := FTotalItemsPopped mod FQueueSize;
    FQueueSize:=FQueueSize+ADelta;
    setlength(FList, FQueueSize);
    if FTotalItemsPopped > FTotalItemsPushed then inc(FTotalItemsPopped, FQueueSize);
  finally
    FMonitor.Leave;
  end;
end;


The other option is to shift all items.
That would benefit "Grow(-10)" which shrinks the queue.
TagsNo tags attached.
Fixed in Revision63811
LazTarget2.2
Widgetset
Attached Files

Activities

Martin Friebe

2020-08-21 02:25

manager   ~0125049

The following should do... / I have to do some testing though

procedure TLazThreadedQueue.Grow(ADelta: integer);
var
  NewList: array of T;
  c: Integer;
  i: QWord;
begin
  FMonitor.Enter;
  try
    c:=Max(FQueueSize + ADelta, FTotalItemsPushed - FTotalItemsPopped);
    setlength(NewList, c);
    i:=FTotalItemsPopped;
    while i < FTotalItemsPushed do begin
      NewList[i div c] := FList[i div FQueueSize];
      inc(i);
    end;

    FList := NewList;
    FQueueSize:=c;
  finally
    FMonitor.Leave;
  end;
end;

Martin Friebe

2020-08-22 22:22

manager   ~0125084

There also is a race condition.

On Windows RTLeventSet, will release one and only one thread waiting with RTLeventWaitFor.


If 2 or more Items are pushed onto the queue, while the threads are in the line marked below (timeout=INFINITE)
- each push will RTLeventSet / But duplicate calls to RTLeventSet still only set the event. (good for one RTLeventWaitFor)
- then all threads will enter RTLeventWaitFor
  one thread gets the event, the others will keep waiting.


TLazThreadedQueue.PopItem
....
EnterCritical
  If item avail then return item
ExitCritical
   // race condition between those lines. <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
RTLeventWaitFor(itemAdded)
EnterCritical
  If item avail then return
ExitCritical


As a remedy, if an event was consumed, the state of the queue is checked (by the thread consuming the event), and the event is set again

Martin Friebe

2020-08-22 22:27

manager   ~0125085

r 63811

Issue History

Date Modified Username Field Change
2020-08-21 00:56 Martin Friebe New Issue
2020-08-21 02:25 Martin Friebe Note Added: 0125049
2020-08-22 02:46 Martin Friebe Assigned To => Martin Friebe
2020-08-22 02:46 Martin Friebe Status new => assigned
2020-08-22 22:22 Martin Friebe Note Added: 0125084
2020-08-22 22:27 Martin Friebe Status assigned => resolved
2020-08-22 22:27 Martin Friebe Resolution open => fixed
2020-08-22 22:27 Martin Friebe Fixed in Version => 2.2
2020-08-22 22:27 Martin Friebe Fixed in Revision => 63811
2020-08-22 22:27 Martin Friebe LazTarget => 2.2
2020-08-22 22:27 Martin Friebe Note Added: 0125085