Image retriever in a thread

I'm trying to make an image retriever to work with a list.

The list contains items of type (TItem) for example. TItem has some properties like title, image and imageURL.

There is a thread with the list that scan all items and try to retrieve the image of each item by using the imageURL of each item.

The thread that retrieve the image of each item work like this :

while not terminated do
begin
 for i := 0 to List.count-1 do
 begin
  item := List.Items[i];
   //Note : it can takes a few sec to retrieve the image from the imageURL. This method 
   //retrieve the image from the item.imageURL and then assign it to item.image
  RetrieveImage(item.imageURL, item.Image);
 end;
 sleep(100);
end;

Unfortunately, it doesn't work in one case : when the list is cleared and that the image of an item is being retrieved by the thread.

(All items reading and writing is protected by a mutex).

What should I do ?

Than开发者_高级运维ks :)

There are many ways to solve this, here are two examples:

• Don't use a list of objects, use a TInterfaceList or a generic list of interfaces. Create an interface from the public methods of the item class. The thread will maintain an interface reference, keeping the reference count above zero and thus the object instance that implements the interface won't be deleted. Accessing the item will therefore be safe.

• Don't directly access the item from your thread, but give only an opaque item handle to the thread. Initially the thread will use that handle to request the data that is necessary to fetch the image, and since it will lock the list the access is safe. When the image is retrieved the thread will use the handle again to set the image to the item in a locked section of code. Should the item not be valid any more the handle will not resolve to an item, and the retrieved image will simply be deleted. You only have to make sure that the handles are not re-used, so for example the list index or the address of the item would both be bad ideas. An integer that will be incremented for each item OTOH would work well.

Simplified code for the second way:

var
  Img: TImage;
  ImgHandle: TImageFromURLHandle;
...

Img := TImage.Create;
try
  while not Terminated do
  begin
    // GetNextImageURL() is thread-safe
    while List.GetNextImageURL(ImgHandle, ImgURL) do begin
      RetrieveImage(ImgURL, Img);
      // SetImage() is thread-safe and will do nothing if the image item
      // is no longer in the list (invalid handle)
      List.SetImage(ImgHandle, Img);
    end; 
    Sleep(100);
  end;
finally
  Img.Free;
end;

You could even use the image URL itself as the handle.

Note that a better way would be to block the thread if the list is empty, your Sleep() call basically is polling. Not much overhead, but still bad style.

The basic problem is that your code does not protect the loop itself using a mutex. As you probably realized, that would make a huge mutex that slows down the system significantly.

Here is a good solution:

• Replace the for-loop with a while-loop
• Create code that finds the next URL, and mutex-protect that code
• Make the image retrieval use variables that do not relate to the list, so that it does not need mutex protection.
• Save the retrieved image by finding the correct index using the URL. This finding and storing must be mutex protected.

Something like this:

while not terminated do
begin
  currenturl:='';
  while true do begin
   Mutex begin
     currenturl:=FindNextUrl(currentUrl);
   Mutex end
   if currenturl='' then break; // No more URLs to be found
   RetrieveImage(currenturl,image);
   Mutex begin
     index:=FindUrlIndex(currenturl)
     List[index].image:=image;
   Mutex end
  end;
  sleep(100);
end;

Add the necessary mutex code, try-statements etc. yourself.