Is it safe to use locking (TMonitor) wihtin Enumerator's constructor/destructor?

Question

I have simple thread safe container class. It has standard add/remove methods. Normally enumerating items is implemented as:

MyList.lock;
try
  // looping here
finally
  MyList.unlock;
end;

But I want to take advantage of for-in support in a thread safe manner:

for item in MyList do 
begin
  // do something
end;

My enumerator implementation locks the container in it's contructor and unlocks it in the destructor. This works but lies on the assumption that Enumerator's instance is created at the beginning of for-in loop and is destroyed at the end. I found that explanation here: How is Enumerator created with for in construction destroyed?

But since locking/unlocking is a critical operation I wonder if this kind of usage is ok?

Here is my implementation:

  TContainer<T> = class
    private
      FPadLock: TObject;
      FItems: TList<T>;
    protected
    public
      type
        TContainerEnumerator = class(TList<T>.TEnumerator)
          private
            FContainer: TContainer<T>;
          public
            constructor Create(AContainer: TContainer<T>);
            destructor Destroy; override;
        end;
      constructor Create;
      destructor Destroy; override;
      procedure add(AItem: T);
      procedure remove(AItem: T);
      function GetEnumerator: TContainerEnumerator;
  end;

{ TContainer<T> }

procedure TContainer<T>.add(AItem: T);
begin
  TMonitor.Enter(FPadLock);
  try
    FItems.Add(AItem);
  finally
    TMonitor.Exit(FPadLock);
  end;
end;

constructor TContainer<T>.Create;
begin
  inherited Create;
  FPadLock := TObject.Create;
  FItems := TList<T>.Create;
end;

destructor TContainer<T>.Destroy;
begin
  FreeAndNil(FItems);
  FreeAndNil(FPadLock);
  inherited;
end;

procedure TContainer<T>.remove(AItem: T);
begin
  TMonitor.Enter(FPadLock);
  try
    FItems.Remove(AItem);
  finally
    TMonitor.Exit(FPadLock);
  end;
end;

function TContainer<T>.GetEnumerator: TContainerEnumerator;
begin
  result := TContainerEnumerator.Create(self);
end;

{ TContainer<T>.TContainerEnumerator }

constructor TContainer<T>.TContainerEnumerator.Create(
  AContainer: TContainer<T>);
begin
  inherited Create(AContainer.FItems);
  FContainer := AContainer;
  TMonitor.Enter(FContainer.FPadLock);  // <<< Lock parent container using Monitor
end;

destructor TContainer<T>.TContainerEnumerator.Destroy;
begin
  TMonitor.Exit(FContainer.FPadLock);  // <<< Unlock parent container
  inherited;
end;

Answer 1

The enumerator is created at the start of the for loop, and destroyed when the loop ends. The lifetime of the enumerator is managed by a try/finally.

Don't just take my word for this though. It's easy enough to add some debugging code that will instrument your loop and let you see when the destructor is called.

This means that your proposed locking strategy is sound.

I would say though that allocating an enumerator on the heap, when you have thread contention, could cause performance problems.