Is this a bug in System.Net.HttpClient on Rio?

This is the function found in Delphi Rio in System.Net.HttpClient

THTTPClientHelper = class helper for THTTPClient
....

procedure THTTPClientHelper.SetExt(const Value);
var
{$IFDEF AUTOREFCOUNT}
  LRelease: Boolean;
{$ENDIF}
  LExt: THTTPClientExt;
begin
  if FHTTPClientList = nil then
    Exit;
  TMonitor.Enter(FHTTPClientList);
  try
{$IFDEF AUTOREFCOUNT}
    LRelease := not FHTTPClientList.ContainsKey(Self);
{$ENDIF}
    LExt := THTTPClientExt(Value);
    FHTTPClientList.AddOrSetValue(Self, LExt);
{$IFDEF AUTOREFCOUNT}
    if LRelease then __ObjRelease;
{$ENDIF}
  finally
    TMonitor.Exit(FHTTPClientList);
  end;
end;

What the guy try to do with LRelease here?

{$IFDEF AUTOREFCOUNT}
    LRelease := not FHTTPClientList.ContainsKey(Self);
{$ENDIF}
    LExt := THTTPClientExt(Value);
    FHTTPClientList.AddOrSetValue(Self, LExt);
{$IFDEF AUTOREFCOUNT}
    if LRelease then __ObjRelease;
{$ENDIF}

So if FHTTPClientList doesn't contain the THTTPClient add it in the FHTTPClientList and then reduce it's refcount by one. Why reduce it's refcount by one?? the THTTPClient is still alive and used why breaking it's refcount? Their is a bug here, maybe the guy make a typo, but i don't understand what he want to do originally ...

for info this this how items are removing from the dictionary :

procedure THTTPClientHelper.RemoveExt;
begin
  if FHTTPClientList = nil then
    Exit;
  TMonitor.Enter(FHTTPClientList);
  try
    FHTTPClientList.Remove(Self);
  finally
    TMonitor.Exit(FHTTPClientList);
  end;
end;

Solution

Purpose of the manual reference counting for ARC compiler in above code is simulating dictionary with weak references. Delphi generic collections are backed with generic arrays that will hold strong reference to any object added to the collection on ARC compiler.

There is several ways to achieve weak references - using pointers, using wrappers around object where object is declared as weak and manual reference counting in appropriate places.

With pointers you lose type safety, wrappers require significantly more code, so I guess the author of above code opted for manual reference counting. Nothing wrong with that part.

However, as you noticed, there is something fishy in that code - while SetExt routine is written properly RemoveExt has a bug resulting in a crash later on.

Let's go through the code in context on ARC compiler (I will omit compiler directives and unrelated code for brevity):

Since adding object into collection (array) increases reference count, to achieve weak reference we have to decrease reference count of the added object instance - that way the instance's reference count will remain the same after it is stored in collection. Next, when we remove object from such collection, we have to restore reference count balance and increase reference count. Also we have to make sure that object will be removed from such collection before it is destroyed - good place to do that is destructor.

Adding to collection:

LRelease := not FHTTPClientList.ContainsKey(Self);
FHTTPClientList.AddOrSetValue(Self, LExt);
if LRelease then __ObjRelease;

We add the object to the collection and then after collection holds strong reference to our object we can release it's reference count. If object is already inside collection that means it's reference count was already decreased and we must not decrease it again - that is the purpose of LRelease flag.

Removing from collection:

if FHTTPClientList.ContainsKey(Self) then
  begin
    __ObjAddRef;
    FHTTPClientList.Remove(Self);
  end;

If object is in collection we have to restore the balance and increase the reference count before removing object from collection. This is the part that is missing from RemoveExt method.

Making sure that object is not in the list upon destruction:

destructor THTTPClient.Destroy;
begin
  RemoveExt;
  inherited;
end;

Note: In order for such faked weak collection to work properly items must be added and removed only through above methods that take care of balancing reference count. Using any other original collection methods like Clear will result in broken reference count.

Bug or not?

In System.Net.HttpClient code broken RemoveExt method is called only in destructor, also FHTTPClientList is private variable and it is not altered in any other way. On the first glimpse, that code works properly, but actually contains rather subtle bug.

To unravel the real bug we need to cover possible usage scenarios, starting with few established facts:

Only methods that alter content and by that reference count of items in FHTTPClientList dictionary are SetExt and RemoveExt methods
SetExt method is correct
Broken RemoveExt method that does not call __ObjAddRef is called only in THTTPClient destructor and this is where this subtle bug originates.

When destructor is called upon any particular object instance that means object instance has reached it's lifetime, and any subsequent reference counting triggers (during destructor execution) have no influence on the code correctness.

That is ensured by applying objDestroyingFlag on FRefCount variable changing its value and any further count increasing/decreasing can no longer result in special value 0 that starts destruction process - so object is safe and will not get destroyed twice.

In above code when THTTPClient destructor is called that means last strong reference to object instance has gone out of scope or was set to nil and at that moment the only remaining live reference that can trigger reference counting mechanism is the one in FHTTPClientList. That reference has been cleared by RemoveExt method (broken or not) at that point as previously said it does not matter. And everything works fine.

But, author of the code has forgotten one tiny weeny thingy - DisposeOf method that triggers the destructor, but at that point object instance has not reached its reference counting lifetime. In other words - if destructor is called by DisposeOf, any subsequent reference counting triggers must be balanced because there are still live references to the object that will trigger reference counting mechanism after the destructor chain calls are completed. If we break the counting at that point result will be catastrophic.

Since THTTPClient is not TComponent descendant that requires DisposeOf it is easy to make the oversight and forget that someone, somewhere could call DipsoseOf on such variable anyway - for instance if you make owned list of THTTPClient instances clearing such list will call DisposeOf on them and happily break their reference count because RemoveExt method is ultimately broken.

Conclusion: Yes, it is a BUG.