-3

Sometimes this function locks my program, and it's freezes until i close it. What is wrong here ?

function del_from_list(id:string):boolean;
var i : integer;
begin
  Result := True;
  try
    with global_list.LockList do
    begin
      for i:=0 to Count-1 do
      begin
        if Tthread_list(Items[i]).id = id then
        begin
          Delete(i);
          break;
        end;
      end;
    end;
  finally
    global_list.UnlockList;
  end;
end;

the class

  Tthread_list = class
  public
    id   : string;
    constructor Create(const id: string);
  end;

I'm adding to the list like that:

global_list.Add(Tthread_list.Create('xxx'));

global list is a global variable

var global_list : TThreadList = nil;
jmp
  • 2,456
  • 3
  • 30
  • 47
  • I see no reason that this should cause a hang. Is this all the code in the app that hangs? – David Heffernan Dec 17 '11 at 08:33
  • 1
    Most likely you have a classic deadlock situation but this code alone cannot cause it. There must be another lock somewhere. – David Heffernan Dec 17 '11 at 09:23
  • You are deleting a class object item in a list without freeing the object first. If an exception occurs because of that, your program will lock. The answer by Remy (which you accepted) solves the lock, but not the primary error. – LU RD Dec 17 '11 at 13:13
  • @LURD so can you explain how Remy's answer solves the problem. I simply must be missing something. – David Heffernan Dec 17 '11 at 14:05
  • @DavidHeffernan, I assumed it solved the problem because it's the accepted answer. – LU RD Dec 17 '11 at 14:11
  • I don't understand how a `list` consists of just a constructor a string field - not even a base class? Unless you only put one small snippet of this class? – Jerry Dodge Dec 17 '11 at 15:15
  • @waza123 you probably should change the name of Tthread_list to something like TListItem because you might confuse people with this naming. Anyway I think downvotes should have a reason. Apart from bad naming I do not understand what people do not like in your question. – Wodzu Nov 06 '12 at 07:10

2 Answers2

5

You need to call LockList() outside of the try block instead of inside of it, eg:

function del_from_list(const id: string): boolean;
var
  List: TList;
  i : integer;
begin
  Result := False;
  List := global_list.LockList;
  try
    with List do
    begin
      for i :=0 to Count-1 do
      begin
        if Tthread_list(Items[i]).id = id then
        begin
          Delete(i);
          Result := True;
          break;
        end;
      end;
    end;
  finally
    global_list.UnlockList;
  end;
end;
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • 1
    Remy, I agree that the resource acquisition (lock) should be done before entering the try..finally block, but doing it the other way (OP) should only cause a problem if the resource acquisition fails and throws. Refresh my memory - can TThreadList.LockList ever throw an exception? – dthorpe Dec 17 '11 at 06:34
  • 5
    @Remy Do you believe that this change, which is indeed correct, will be the difference between a working app and a deadlock? If so can you explain why you believe that. If not then why did you post this as an answer? It seems like it should be a comment to me. – David Heffernan Dec 17 '11 at 08:52
  • @dthorpe LockList today is `TMonitor.Enter(FLock); Result := FList;` so it won't throw unless there is heap corruption or a bug in `TMonitor`. – David Heffernan Dec 17 '11 at 12:12
  • From the documentation of TList.Delete : `Note: Delete does not free any memory associated with the item.` I think the primary error here is the missing `Tthread_list(Items[i]).Free`. – LU RD Dec 17 '11 at 13:20
  • 3
    @LU RD That's just a memory leak. It's not going to lead to a deadlock. – David Heffernan Dec 17 '11 at 13:58
  • @DavidHeffernan, I'm still puzzled how Remy's answer could solve the deadlock. Yes, TMonitor could be the cause, but stepping through TMonitor source code in this example I can't find any weak points. – LU RD Dec 17 '11 at 14:08
  • @dthorpe: I hadn't noticed that `TThreadList` was updated to use `TMonitor`. It used to use a `TCriticalSection` instead. That worries me, considering that `TMonitor` has known issues that have been blogged about several times. – Remy Lebeau Dec 17 '11 at 17:09
  • @Remy Do you have any explanation yet as to why your answer would fix a deadlock? – David Heffernan Dec 17 '11 at 18:15
  • As to switch to `TMonitor`, is that because critical sections are Windows only and `TThreadList` is x-plat? – David Heffernan Dec 17 '11 at 18:17
0

for loop counts in the wrong direction. When deleting members, you MUST count down, not up.

Dennis Galvin
  • 21