Is delphi TQueue buggy? Using TQueue<Tbytes> return nil with dequeue

Question

I don't understand why this very simple code failed? I'm on Delphi Tokyo release 2.

{$APPTYPE CONSOLE}

uses
  System.SysUtils,
  System.Generics.Collections;

procedure Main;
var
  aQueue: TQueue<TBytes>;
  aBytes: TBytes;
begin
  aQueue := TQueue<TBytes>.create;

  aBytes := TEncoding.UTF8.GetBytes('abcd');
  aQueue.Enqueue(aBytes);
  aBytes := aQueue.Dequeue;
  Writeln(Length(aBytes)); // outputs 4 as expected

  aBytes := TEncoding.UTF8.GetBytes('abcd');
  aQueue.Enqueue(aBytes);
  aBytes := aQueue.Dequeue;
  Writeln(Length(aBytes)); // outputs 0
end;

begin
  Main;
  Readln;
end.

Is this a bug?

NOTE: The code works correctly on XE4, but fails also on Berlin.

Agonizing

the guy who vote -1, can he share in the comment why he vote -1 ? same for the other who will do the same ... – Agonizing

no no, it's this exact code (copy past on a blank new project) ! nothing else ... i m on delphi tokyo release 2 – Agonizing

@DavidHeffernan : on with version of tokyo you are ? – Agonizing

I'm not using Tokyo, but the question did not state a specific Delphi version. – Flogging

ok, so on tokyo it's failed, i just try to launch it on xe4 and on xe4 it's work ... someone can confirm ? – Agonizing

Btw, a proper minimal reproducible example could be compiled directly. Make it with a console app and output using Writeln. I will edit your question to demonstrate. – Flogging

@DavidHeffernan : i try on berlin, it's failed also on berlin ... what version are you using ? – Agonizing

FWIW, your code works correctly also in XE7. You need to visit the quality portal of the supplier, and file an error report, if it isn't there already. – Ludovico

@David, glad you finally received an MCVE. Now, could you think more about searchability of this question? About the way how to search it by the compiler error message? Or do you think that someone is able to find their problem by "Is delphi TQueue buggy?" Not speaking that it doesn't contain what is wrong with the code and as such can be closed. If you are asking for why I say this; it is because you are many times strict just for an MCVE but may not think much about the question in general ;-) vostock, it was me because of what I said in this comment (fix it and I'll change). – Endocrinotherapy

@Victoria: I updated the title so you can revert your vote now ;) but admit writing -1 without saying anything is just wasting of time ... at least david say because he can't reproduce it and thus help a lot to understand that 1) this was really a bug and 2) don't affect all version. how many developer i advise to come here, they ask a question and get -x on their first question without any comment, get immediately blacklisted and never come here again thinking this community is useless (or made of savant monkey as you prefer) ... on this point so is totally absurd or stupid as you prefer – Agonizing

@vostock, vote removed. But it's still a closable question. Think about a question like this though. There is no MCVE and I wasn't able to reproduce because of my fault. Yet it's searchable for others and clearly describes the problem. That's a question that deserves more than your score. If you added even what compiler rejects, I would even upvote.. But thank you anyway! ;-) – Endocrinotherapy

@Victoria: Sometime i think too much verbosity kill the question... thanks to you too ! – Agonizing

@vostock, thank you for that! Me and many others here appreciate that, I guess. But try to think also about searchability for the others with the same problem as well ;-) – Endocrinotherapy

@Victoria: i agree but unfortunately google (sick please everyone stop using google) will be soon able to do this job for you ... try this as example : cloud.google.com/vision post any picture and he can found the number of cake in it, if the woman is nude or doing something sexy, the type of car, the plane, the tree, etc. if you gave a picture of a cake it's close to gave you the number of calories! IA is coming, fast ... in the next 5 years we will not care anymore of choosing good keyword ... – Agonizing

Yikes. I'm used to environments where "is the standard library buggy" questions are normally closed with a "no, it's you". – Gregale

@chrylis welcome to delphi – Flogging

@chrylis, yes, welcome to Delphi tag. Where double meter is used by users. Some questions doesn't even need to include problem description to be highly rated whilst some are downvoted by the same users for pointless details :) – Endocrinotherapy

@Endocrinotherapy You are right about the title, I should have improved that when editing. But you also could have done so. – Flogging

Answer 1

This is indeed a bug. The code works correctly in XE7, but not XE8. In XE8 the output is 0 for both attempts.

Certainly the XE8 generic collections were very buggy and subsequent releases fixed many of the defects. Clearly not all have been fixed.

In XE8 Embarcadero attempted to address the issue of generic bloat caused by weaknesses in their compile/link model. Unfortunately, instead of tackling the problem at the root, they chose instead to address the issue in the library code for generic collections. Doing so they completely broke many of the generic collection classes, proving that their unit testing was weak. And of course, by addressing the problem this way they failed to address the issue of generic bloat for classes other than those in the generic collections. All in all, a sorry story that is seemingly still not over.

loki has just submitted a bug report: RSP-20400.

Note that this bug report is incorrect because (at least according to Stefan Glienke) the bug has been fixed in Tokyo 10.2.3. So upgrading to 10.2.3 should be the simplest way to resolve the problem.

Perhaps this bug report is more appropriate: RSP-17728.

Writing a generic queue isn't even difficult. Here's one that is known to work:

type
  TQueue<T> = class
  private
    FItems: TArray<T>;
    FCount: Integer;
    FFront: Integer;
  private
    function Extract(Index: Integer): T; inline;
    function GetBack: Integer; inline;
    property Back: Integer read GetBack;
    property Front: Integer read FFront;
    procedure Grow;
    procedure RetreatFront; inline;
  public
    property Count: Integer read FCount;
    procedure Clear;
    procedure Enqueue(const Value: T);
    function Dequeue: T;
    function Peek: T;
  public
    type
      TEnumerator = record
      private
        FCollection: TQueue<T>;
        FCount: Integer;
        FCapacity: Integer;
        FIndex: Integer;
        FStartIndex: Integer;
      public
        class function New(Collection: TQueue<T>): TEnumerator; static;
        function GetCurrent: T;
        property Current: T read GetCurrent;
        function MoveNext: Boolean;
      end;
  public
    function GetEnumerator: TEnumerator;
  end;

function GrownCapacity(OldCapacity: Integer): Integer;
var
  Delta: Integer;
begin
  if OldCapacity>64 then begin
    Delta := OldCapacity div 4
  end else if OldCapacity>8 then begin
    Delta := 16
  end else begin
    Delta := 4;
  end;
  Result := OldCapacity + Delta;
end;

{ TQueue<T> }

function TQueue<T>.Extract(Index: Integer): T;
begin
  Result := FItems[Index];
  if IsManagedType(T) then begin
    Finalize(FItems[Index]);
  end;
end;

function TQueue<T>.GetBack: Integer;
begin
  Result := Front + Count - 1;
  if Result>high(FItems) then begin
    dec(Result, Length(FItems));
  end;
end;

procedure TQueue<T>.Grow;
var
  Index: Integer;
  Value: T;
  Capacity: Integer;
  NewItems: TArray<T>;
begin
  Capacity := Length(FItems);
  if Count=Capacity then begin
    SetLength(NewItems, GrownCapacity(Capacity));
    Index := 0;
    for Value in Self do begin
      NewItems[Index] := Value;
      inc(Index);
    end;

    FItems := NewItems;
    FFront := 0;
  end;
end;

procedure TQueue<T>.RetreatFront;
begin
  inc(FFront);
  if FFront=Length(FItems) then begin
    FFront := 0;
  end;
end;

procedure TQueue<T>.Clear;
begin
  FItems := nil;
  FCount := 0;
end;

procedure TQueue<T>.Enqueue(const Value: T);
begin
  Grow;
  inc(FCount);
  FItems[Back] := Value;
end;

function TQueue<T>.Dequeue: T;
var
  Index: Integer;
begin
  Assert(Count>0);
  Result := Extract(Front);
  RetreatFront;
  dec(FCount);
end;

function TQueue<T>.Peek: T;
begin
  Assert(Count>0);
  Result := FItems[Front];
end;

function TQueue<T>.GetEnumerator: TEnumerator;
begin
  Result := TEnumerator.New(Self);
end;

{ TQueue<T>.TEnumerator }

class function TQueue<T>.TEnumerator.New(Collection: TQueue<T>): TEnumerator;
begin
  Result.FCollection := Collection;
  Result.FCount := Collection.Count;
  Result.FCapacity := Length(Collection.FItems);
  Result.FIndex := -1;
  Result.FStartIndex := Collection.Front;
end;

function TQueue<T>.TEnumerator.GetCurrent: T;
var
  ActualIndex: Integer;
begin
  ActualIndex := (FStartIndex + FIndex) mod FCapacity;
  Result := FCollection.FItems[ActualIndex];
end;

function TQueue<T>.TEnumerator.MoveNext: Boolean;
begin
  inc(FIndex);
  Result := FIndex<FCount;
end;

Flogging

Thanks david, you confirm what i was thinking ... but how i can make it working in the middle time ? – Agonizing

My advice is to use a collection library that works. I know I don't use the RTL generic collections and have written my own. That's a big task, so I would recommend Spring4D's collections. – Flogging

This really makes me consider downgrading our projects from 10 Seattle back to XE7. – Electroencephalograph

@GünthertheBeautiful I've never updated our company product beyond XE7 for that reason. I'm also now completely distanced from Generics.Collections, my codebase does not refer to it once. – Flogging

I find a better way to get ride of this bug: use AnsiString instead of TBytes :) in fact their is no real reason to make a difference between then (except when we want to make everything complicated) ... – Agonizing

you can not get ride of Generics.Collections :( it's used in more than 482 unit in the delphi source code ... it's part of the root of delphi :( – Agonizing

Using AnsiString is a bad idea in my view. You've got many better solutions than that. – Flogging

If (and I'm saying IF not WHEN) you are going down the route of storing binary data in a string, you should at least use RawByteString instead of AnsiString... But generally one should refrain from using strings to store binary data... – Frasch

Answer 2

To add to David's answer, the bug is in the Enqueue method. The top branch should be handling all reference counted managed types.

  if IsManagedType(T) then
    if (SizeOf(T) = SizeOf(Pointer)) and (GetTypeKind(T) <> tkRecord) then
      FQueueHelper.InternalEnqueueMRef(Value, GetTypeKind(T))
    else
      FQueueHelper.InternalEnqueueManaged(Value)
  else

But here we see that dynamic arrays are conspicuously missing in InternalEnqueueMref, which falls through without doing anything:

procedure TQueueHelper.InternalEnqueueMRef(const Value; Kind: TTypeKind);
begin
  case Kind of
    TTypeKind.tkUString: InternalEnqueueString(Value);
    TTypeKind.tkInterface: InternalEnqueueInterface(Value);
{$IF not Defined(NEXTGEN)}
    TTypeKind.tkLString: InternalEnqueueAnsiString(Value);
    TTypeKind.tkWString: InternalEnqueueWideString(Value);
{$ENDIF}
{$IF Defined(AUTOREFCOUNT)}
    TTypeKind.tkClass: InternalEnqueueObject(Value);
{$ENDIF}
  end;
end;

It's so egregious, in fact, that the compiler actually produces no code for Enqueue when compiled (other than preamble) since the futility of the exercise can be determined from the types at compile time.

Project1.dpr.15: aQueue.Enqueue(aBytes);
0043E19E 8B45F8           mov eax,[ebp-$08]
0043E1A1 8945F4           mov [ebp-$0c],eax
0043E1A4 8B45FC           mov eax,[ebp-$04]
0043E1A7 83C008           add eax,$08
0043E1AA 8945F0           mov [ebp-$10],eax
Project1.dpr.16: aBytes := aQueue.Dequeue;
0043E1AD 8D45EC           lea eax,[ebp-$14]

This bug, therefore, would be expected to affect TQueue<T> for T being any type of dynamic array.

Craiova

ffs this is the same family of bugs introduced in XE8. Are they just waiting for us to submit reports before they fix them? What about a proactive code review and introduction of comprehensive tests? – Flogging

@DavidHeffernan I like your sense of humour. – Craiova

the compiler actually produces no code for Enqueue when compiled 😭 – Flogging

@DavidHeffernan Well... it's good for the day's entertainment, at least. – Craiova

"no code for Enqueue": On the positive: it shows that the new intrinsics really work. <g> – Appreciable