Multithreaded file upload synchronization

This may not be the problem, but TFileInfo is a record.

This means that when passed as a (non const/var) parameter, it gets copied. This can result in issues with things like strings in the record which don't get reference counts updated when the record is copied.

One thing to try would be to make it a class and pass an instance as the parameter (i.e. a Pointer to the data on the heap).

Something else to watch out for is shared Int64's (e.g. your size values) on threaded 32bit systems.

Updating/reading these is not done atomically & you don't have any specific protections, so it is possible for a read of the value to get mismatched upper and lower 32-bits due to threading. (e.g. Read Upper 32 bits, Write Upper 32bits, Write lower 32bits, Read Lower 32bits, with reads & write in different threads). This is probably not causing the problems you are seeing and unless you are working with files transfers of > 4GB, unlikely to ever cause you any issues.

Deadlocks are definitely hard to spot, but this may be the problem.In your code, I didn't see that you added any timeout to the enqueue, peek or dequeue - which means it will take the default of Infinite.

The enqueue has this line in it - meaning, like any synchronization object, it will block until either the Enter completes (it locks the monitor) or the Timeout occurs (since you don't have a timeout, it will wait forever)

TSimpleThreadedQueue.Enqueue(const Item: T; Timeout: LongWord): TWaitResult;...    if not TMonitor.Enter(FQueue, Timeout)

I'm also going to make the assumption that you implemented PEEK yourself based on the Dequeue - only you don't actually remove the item.

That appears to implement its own timeout - however, you still have the following:

function TSimpleThreadedQueue.Peek/Dequeue(var Item: T; Timeout: LongWord): TWaitResult;...if not TMonitor.Enter(FQueue, Timeout)

Where timeout is Infinite - so, if you are in the peek method waiting for it to be signaled with an infinite timeout, then you can't Enqueue something from a second thread without blocking that thread waiting for the peek method to become complete on an infinite timeout.

Here is a snippet of the comment from TMonitor

Enter locks the monitor object with an optional timeout (in ms) value. Enter without a timeout will wait until the lock is obtained. If the procedure returns it can be assumed that the lock was acquired. Enter with a timeout will return a boolean status indicating whether or not the lock was obtained (True) or the attempt timed out prior to acquire the lock (False). Calling Enter with an INFINITE timeout is the same as calling Enter without a timeout.

Since the implementation uses Infinite by default, and a TMonitor.Spinlock value is not provided, that will block the thread until it can acquire the FQueue object.

My suggestion would be to change your code as follows:

  // Use Peek instead of Dequeue, because the item should not be removed from the queue if it fails  while true do    case fQueue.Peek(fCurrentFile,10)       wrSignaled:        try          if UploadFile(fCurrentFile) = '' then          begin            fQueue.Dequeue(Temp);  // Delete the item from the queue if succesful            SignalComplete;          end;        except          on E: Exception do            SignalError(E.Message);        end;      wrTimeout: sleep(10);      wrIOCompletion,      wrAbandoned,      wrError: break;    end; //case

This way, peek won't hold the lock on FQueue indefinitely, leaving a window for the Enqueue to acquire it and add the file from the main (UI) thread.

This might be a long shot, but here is another possibility [the former answer may be more likely] (something I just ran across, but had known before): The use of Synchronize may be causing the deadlock. Here is a blog about why this happens:Delphi-Workaround-for-TThread-SynchronizeWaitFor-.aspx

The pertinent point from the article:

Thread A calls Synchronize(MethodA)

Thread B calls Synchronize(MethodB)

Then, inside the context of the Main Thread:

Main thread calls CheckSynchronize() while processing messages

CheckSynchronize is implemented to batch-process all waiting calls(*). So it picks up thequeue of waiting calls (containing MethodA and MethodB) and loopsthrough them one by one.

MethodA executes in the main thread'scontext. Assume MethodA calls ThreadB.WaitFor

WaitFor callsCheckSynchronize to process any waiting calls to Synchronize

In theory, this should then process ThreadB's Synchronize(MethodB),allowing Thread B to complete. However, MethodB is already apossession of the first CheckSynchronize call, so it never getscalled.

DEADLOCK!

Embarcadero QC article describing the problem in more detail.

While I don't see any ProcessMessages calls in the above code, or for that matter, a WaitFor that would be called during a Synchronize, it could still be a problem that at the point a synchronize is called, another thread calls the synchronize as well - but the main thread has already synchronized and is blocking.

This didn't click with me at first, because I tend to avoid Synchronize calls like the plague and usually design UI updates from threads using other methods like message passing and thread safe lists with message notification instead of synchronize calls.