TThread: Problem mit Terminate(), direkt gefolgt von WaitFor()



  • Ich hab hier ein Problem mit dem Beenden von Threads, die intern Speicher reservieren und also auch wieder freigeben müssen. Bei FreeOnTerminate=true wird bei dem TestThread der __finally-Block nicht durchlaufen, wenn Thread::Terminate() aufgerufen wird, nachdem die Beendigung der Anwendung eingeleitet wurde. Sprich ein Thread::Teminate() im Destruktor des Hauptformulars führt zu einem sofortigen Abbruch des Threads, ohne das der reservierte Speicher freigegeben wird.

    Anhand dieses simplen Threads läßt sich das Problem nachvollziehen:

    __fastcall TTestThread::TTestThread(bool CreateSuspended)
    	: TThread(CreateSuspended)
    {
    	FreeOnTerminate = false;
    	Priority = tpLowest;
    	Resume();
    }
    //---------------------------------------------------------------------------
    
    void __fastcall TTestThread::Execute()
    {
    	char* cTest = NULL;
    	try
    	{
    		try
    		{
    			while (!Terminated)
    			{
    				cTest = new char[100];
    				Sleep(500); // Simulation irgendwelcher Aufgaben...
    				delete[] cTest;
    				cTest = NULL;
    			}
    		}
    		catch(Exception& Error)
    		{
    			delete[] cTest;
    		}
    	}
    	__finally
    	{
    		delete[] cTest;
    	}
    }
    //---------------------------------------------------------------------------
    

    Thread wird wie so beendet;

    TestThread->Terminate();
    TestThread->WaitFor();
    delete TestThread;
    TestThread = NULL;
    

    Das führt dazu, dass die gesamte Anwendung für 5 bis 15 Sekunden, mit fast 100% CPU-Last bei dem WaitFor() 'hängt'.

    Interessanterweise funktioniert es mit einem Sleep(600) zwischen dem Terminate() und dem WaitFor() praktisch klaglos. Aber ich kann doch nicht für jeden Thread berechnen, wie lange er braucht, um die Execute-Methode abzuschließen, nachdem Terminate() ausgelöst wurde, um ein entsprechendes Sleep() einzubauen?!? Mache ich da irgendwo einen dummen Fehler??

    Der Beispiel-Thread entspricht dem tatsächlichen Thread, nur das da kein Sleep() drin ist, sondern ein ReadDirectoryChangesW mit einem entsprechenden WaitForSingleObject.

    Es gibt 4 zu übewachende Verzeichnisse, was die Situation nicht gerade verbessert. Zur Zeit bediene ich mich des Workarounds, dass ich im FormCloseQuery, die Threads termininiere, anschließend ein Sleep ausführe und im Destruktor nur noch WaitFor() und delete ausgeführe. Das gefällt mir allerdings überhaupt nicht...



  • Dürfte ich mal den echte Code sehen? Vor Allem die WaitForSingleObject würde mich interessieren.
    Ins Blaue geraten, würde ich behaupten, dein Thread wartet, bis ein Timeout in WaitFor SingleObject passiert.

    Ich habe derlei Probleme immer folgendermassen umgangen:

    • Ein klasseneigenes Array des Typs HANDLE anlegen, welches alle Handles beinhaltet auf die der Thread warten soll. (also auch das von ReadDirectoryChanges z.B.)
    • im Konstruktor habe ich dann immer das Event 0 mit dem Handle für ein mit CreateEvent erstelltes Handle befüllt, den rest mit den sonstigen WaitFor*Objects-Events.
    • Dann habe ich die Terminate-Funktion überschrieben, sodass sie vor dem Aufruf der Basisfunktion mit SetEvent das Event welches ich selber im Konstruktor erstellt habe auslöst. (das TerminateAlert-Event sozusagen)
    • Bei WaitForMultipleObjects den Rückgabewert so ausgewertet, dass ich einfach noch alles was zu erledigen ist vor dem Verlassen der Execute noch erledigt wird. Dann rausspringen. (Lässt sich schön mit Switch/Case erledigen)
    • Im Destruktor dann natürlich aufräumen (z.B: CloseHandle auf das Eventhandle, etc)

    Damit kam ich eigentlich immer ganz gut durch. In dieser Hinsicht ist TThread leider etwas unglücklich gebaut.

    [edit="junix"]Mit WaitForMultipleObjects lässt sich im Übrigen zum Beispiel auch das Überwachen mehrerer Verzeichnisse über einen Thread wunderbar erschlagen.... fällt mir nur jetzt grad ein. Kommt aber natürlich auf deinen Applikationsdesign an. [/edit]



  • Hallo junix,

    sorry, das ich jetzt erst antworte, aber es war Rennwochenende. Da wird nicht programmiert... 😉

    Wie gewünscht erst mal die Routine

    Header:

    #ifndef ThreadMonitorDirH
    #define ThreadMonitorDirH
    //---------------------------------------------------------------------------
    #include <Classes.hpp>
    //---------------------------------------------------------------------------
    
    // Forward Declarations
    class cMonitoredDirectory;
    
    class TMonitorDirThread : public TThread
    {            
    private:
    	cMonitoredDirectory* mdOwner;
    	AnsiString asDir;
    	HANDLE FDirectoryHandle;
    	char* fInfo;
    	DWORD FBytesWritten, FNotifyFilter, bufsize;
    	OVERLAPPED FOverlapped;
    	bool bInternalError;
    	TStringList* slInternalBuffer;
    protected:
    	void __fastcall Execute();
    public:
    	__fastcall TMonitorDirThread(bool CreateSuspended, cMonitoredDirectory* MonitoredDirectory);
    	__fastcall ~TMonitorDirThread();
    };
    //---------------------------------------------------------------------------
    #endif
    

    CPP:

    #include <vcl.h>
    #pragma hdrstop
    
    #include "ThreadMonitorDir.h"
    #include "DataObjects.h"
    #pragma package(smart_init)
    //---------------------------------------------------------------------------
    
    __fastcall TMonitorDirThread::TMonitorDirThread(bool CreateSuspended, cMonitoredDirectory* MonitoredDirectory)
    	: TThread(CreateSuspended)
    {
    	mdOwner = MonitoredDirectory;
    	mdOwner->Lock->BeginRead();
    	asDir = IncludeTrailingPathDelimiter(mdOwner->MonitoredDir);
    	mdOwner->Lock->EndRead();
    	FDirectoryHandle = NULL;
    	fInfo = NULL;
    	bufsize = 4096;
    	slInternalBuffer = new TStringList();
    	bInternalError = false;
    }
    //---------------------------------------------------------------------------
    
    __fastcall TMonitorDirThread::~TMonitorDirThread()
    {
    	if (FDirectoryHandle != NULL && FDirectoryHandle != INVALID_HANDLE_VALUE)
    		CloseHandle(FDirectoryHandle);
    	delete[] fInfo;
    	delete slInternalBuffer;
    	TThread::inherited();
    }
    //---------------------------------------------------------------------------
    
    void __fastcall TMonitorDirThread::Execute()
    {
    	try
    	{
    		try
    		{
    			while (true)
    			{
    				fInfo = new char[bufsize];
    
    				FNotifyFilter = FILE_NOTIFY_CHANGE_LAST_WRITE;
    				FDirectoryHandle = CreateFile(asDir.c_str(),
    														FILE_LIST_DIRECTORY, FILE_SHARE_READ |
    														FILE_SHARE_WRITE | FILE_SHARE_DELETE,
    														NULL,
    														OPEN_EXISTING,
    														FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED,
    														0);
    				if (FDirectoryHandle == INVALID_HANDLE_VALUE)
    				{
    					FDirectoryHandle = NULL;
    					bInternalError = true;
    					break;
    				}
    				ZeroMemory(&FOverlapped, sizeof(OVERLAPPED));
    				ZeroMemory(fInfo, bufsize);
    				FBytesWritten = 0;
    
    				if (ReadDirectoryChangesW(FDirectoryHandle, fInfo, bufsize,	false, FNotifyFilter, &FBytesWritten, &FOverlapped, NULL) > 0)
    				{
    					if(WaitForSingleObject(FDirectoryHandle, 200) == WAIT_OBJECT_0)
    					{
    						if (HasOverlappedIoCompleted(&FOverlapped))
    						{
    							FILE_NOTIFY_INFORMATION* pTemp = (FILE_NOTIFY_INFORMATION*) fInfo;
    							if (pTemp->Action == FILE_ACTION_ADDED || pTemp->Action == FILE_ACTION_MODIFIED)
    							{
    								slInternalBuffer->Add(pTemp->FileName);
    							}
    						}
    					}
    					else
    					{
    						if (slInternalBuffer->Count)
    						{
    							bool bDataTransmitted = false;
    							try
    							{
    								mdOwner->Lock->BeginWrite();
    								for (int i = 0; i < slInternalBuffer->Count; i++)
    									mdOwner->AddFile(slInternalBuffer->Strings[i]);
    								bDataTransmitted = true;
    							}
    							__finally
    							{
    								mdOwner->Lock->EndWrite();
    								if (bDataTransmitted)
    									slInternalBuffer->Clear();
    							}
    						}
    					}
    
    				}
    				else
    				{
    					bInternalError = true;
    					break;
    				}
    				if (FDirectoryHandle != NULL && FDirectoryHandle != INVALID_HANDLE_VALUE)
    					CloseHandle(FDirectoryHandle);
    				FDirectoryHandle = NULL;
    				delete[] fInfo;
    				fInfo = NULL;
    				if (Terminated || bInternalError)
    					break;
    			}
    		}
    		catch (Exception &Ex)
    		{
    			delete[] fInfo;
    			fInfo = NULL;
    			delete slInternalBuffer;
    			slInternalBuffer = NULL;
    			if (FDirectoryHandle != NULL && FDirectoryHandle != INVALID_HANDLE_VALUE)
    				CloseHandle(FDirectoryHandle);
    			bInternalError = true;
    		}
    	}
    	__finally
    	{
    		if (bInternalError && !Terminated)
    		{
    			mdOwner->Lock->BeginWrite();
    			mdOwner->MonitorDirThreadDied();
    			mdOwner->Lock->EndWrite();
    		}
    	}
    }
    //---------------------------------------------------------------------------
    

    Es ist natürlich vollkommen richtig, dass mein Thread im WaitForSingleObject auf einem Timeout wartet, bevor er die Beendigung einleitet. Aber ich dachte 200 ms wären ein vertretbarer Timeout-Zeitraum. Offensichtlich nicht.

    Ansonsten werde ich jetzt gleich mal Deinen Vorschlag umsetzen.
    Und wenn ich da schon WaitForMultipleObjects einsetze, spricht tatsächlich nichts dagegen, mehrere Verzeichnisse in einem Thread zu überwachen.

    Danke.



  • Nun ja, ganz so einfach war es dann doch nicht... Das sofortige Reagieren auf Terminate() hatte leider nicht den gewünschten Effekt. Die hohe CPU-Last beim WaitFor() blieb trotzdem.
    Ich hab jetzt schlußendlich zwei Evnts pro Thread eingesetzt. Mit dem einen signalisiert die Anwendung dem Thread, das er beendet werden soll (WaitForSingleObject durch WaitForMultipleObjects ersetzt) und einen, der im Thread ausgelöst wird, sobald er die Aufräumarbeiten abgeschlossen hat (wird als letztes im __finally-Block). Die Anwendung wartet, ebenfalls per WaitForMultipleObjects, bis alle Threads diesen Event ausgelöst haben und beginnt dann erst die Threads zu löschen. Funktioniert so weit ganz gut. Und das wichtigste: Alle Ressourcen werden nun endlich korrekt freigegeben.

    Außerdem hab ich ein bißchen was über die Win-API gelernt. Semaphores sind wirklich praktisch und CRITICAL_SECTION hat das, was ich bei TCriticalSection vermißt habe: TryEnterCriticalSection...



  • Joe_M. schrieb:

    Nun ja, ganz so einfach war es dann doch nicht... Das sofortige Reagieren auf Terminate() hatte leider nicht den gewünschten Effekt. Die hohe CPU-Last beim WaitFor() blieb trotzdem.

    Hmmm, ja. Ich glaube, die Thread-Klasse ist nicht übermässig geschickt implementiert in diesem Punkt und wartet effektiv auf die Rückkehr der Run-Funktion. Und offensichtlich in einem loop statt per WaitFor*Objects. Kann es sein, dass deine Aufräumaktion relativ lang dauert?

    Joe_M. schrieb:

    Ich hab jetzt schlußendlich zwei Evnts pro Thread eingesetzt. Mit dem einen signalisiert die Anwendung dem Thread, das er beendet werden soll (WaitForSingleObject durch WaitForMultipleObjects ersetzt) und einen, der im Thread ausgelöst wird, sobald er die Aufräumarbeiten abgeschlossen hat (wird als letztes im __finally-Block). Die Anwendung wartet, ebenfalls per WaitForMultipleObjects, bis alle Threads diesen Event ausgelöst haben und beginnt dann erst die Threads zu löschen.

    Klingt nach einem vernünftigen Ansatz... Einzig etwas Optimierpotential bietet das Spiel noch: Du könntest das Resultat von WaitForMultipleObjects auswerten und den auslösenden Thread entsprechend gleich killen. Dann hast du wieder etwas Parallelarbeit.

    Joe_M. schrieb:

    Außerdem hab ich ein bißchen was über die Win-API gelernt. Semaphores sind wirklich praktisch und CRITICAL_SECTION hat das, was ich bei TCriticalSection vermißt habe: TryEnterCriticalSection...

    Schön, wenn jemand die nicht pfannenfertigen Lösungen auch mal positiv auffasst (o;



  • junix schrieb:

    Kann es sein, dass deine Aufräumaktion relativ lang dauert?

    Nein, die Aufräumarbeiten beschränken sich auf:

    if (FDirectoryHandle != NULL && FDirectoryHandle != INVALID_HANDLE_VALUE)
       CloseHandle(FDirectoryHandle);
    delete[] fInfo;
    

    und ähnliche einfache Operationen.
    Und das Event wird erst ausgelöst, nachdem dies geschehen ist. Praktisch als letzte Funktion im Thread. Dennoch geht die CPU-Last für mehrere Sekunden auf 100%, wenn ich sofort danach, WaitFor() aufrufe. Setze ich ein Sleep(50) davor ist alles ok, die WaitFor-Funktion kehrt dann sofort zurück, ohne CPU-Last zu erzeugen. Alle Threads laufen allerdings mit Priority = tpLowest, weil die Anwendung nur wenig CPU-Zeit verbrauchen darf. Die läuft später auf Servern, die praktisch permanent am Anchlag laufen. Ich hätte mich durchaus mit 40 Sekunden zum Beenden der Anwedung abfinden können, wenn nur die CPU-Last nicht so dermaßen ansteigen würde.

    junix schrieb:

    Du könntest das Resultat von WaitForMultipleObjects auswerten und den auslösenden Thread entsprechend gleich killen. Dann hast du wieder etwas Parallelarbeit.

    Das ist ja der Witz daran, das ist überhaupt nicht mehr nötig. So wie es jetzt ist, werden die Threads, beim Beenden der Anwendung, verzögerungsfrei gelöscht.
    Das Problem scheint zu sein, dass man WaitFor() (oder auch delete IrgendeinThread) nicht sofort aufrufen darf, nachdem der Thread seine Execute-Methode abgeschlossen hat, sondern eine minimale Verzögerung einbauen muß. Es ist wohl wie Du schon gesagt hast, schlampig implemtiert...
    Das WaitForMulti... wartet tatsächlich auf die Rückmeldung aller Threads, bevor es weitergeht. Dadurch, dass ich den Thread, der am längsten zur Rückmeldung braucht (der speichert noch was ab), als letzten mit WaitFor() auswerte, liegt bei allen Threads eien minimale Verzögerung zwischen ihrer Beendigung und dem WaitFor().

    junix schrieb:

    Schön, wenn jemand die nicht pfannenfertigen Lösungen auch mal positiv auffasst (o;

    So was nenn ich 'Aus der Not eine Tugend machen'. 😉

    Das ganze ist mittlerweile auch ein bißchen komplexer geworden, als hier dargestellt. Nach diesen dummen Problemen die ich hatte, habe ich das Anwendungsdesign komplett umgestellt und die Arbeiten auf diverse Threads aufgedröselt, um zu prüfen, ob irgendeine Teilarbeit das Problem verursacht...
    Die Hauptanwendung besteht praktisch nur noch aus dem Frontend und kommuniziert ausschließlich mit dem 'Controller' getauften Thread. Dieser wiederum ist für die Koordination der restlichen Threads zuständig. Ein Thread ist nur für die Überwachung der Verzeichnisse zuständig. Er schreibt die Namen der geänderten Dateien in ein Interface-Objekt.Dann meldet er die Änderung an den Controller (hier läßt sich hervorragend ein Semaphore-Objekt einsetzen 😉 ). Der Controller liest die Dateinamen aus dem Interface und stellt sie in die interne Warteschlange. Sobald sich was in der Warteschlange befindet, wird ein 'LogReader'-Thread erzeugt, der die Datei auswertet. Die Ergebnisse stellt er in ein Interface-Objekt. Seioe Execute-Methode wird abgeschlossen und er sendet das entsprechende Event an den Controller. Der holt die Ergebnisse aus dem Interface und stellt sie in das Interface für den 'ResultCollector'. Dann löst der Controller diesmal ein Semaphore aus und der ResultCollector liest die Daten aus dem Interfache, sortiert diese ein und lädt die geänderten oder neuen Datensätze stündlich auf einen FTP-Server hoch. Dort landen die Daten von 3 Servern... Bis auf den FTP-Teil ist es soweit funktionsfähig.
    Es ist eigentlich ein Witz, welchen Aufwand ich hier betrieben habe, nur um dann festzustellen, das es gar nicht an mir lag...
    Nun gut, jetzt bleibt es so, es läuft seht gut, so wie es jetzt ist und auch wenn man den Controller, den LogReader und den Resultcollector wieder zusammenfassen könnte.


Anmelden zum Antworten