memory-leaks bei Verwendung von Threads

AndyDD

ich komm grad nicht weiter bei einem Thread-Problem. In meiner SDI-Anwendung gibt es einen Dialog, der einen Button hat. Nach dem Buttondruck wird ein Thread gestartet, der Berechnungen durchführt. Innerhalb des Dialoges gibt es zwei Membervariablen. Die eine m_bIsRuning wird true gesetzt sobald der Thread startet. Die zweite m_bWantExit soll dem Thread mitteilen, dass er sich beenden soll, falls man ihn vorher abbrechen will. Das funktioniert auch sehr gut. Damit ich den Thread nicht mehrfach starten kann disable ich den Button und enable ihn erst, bevor die Threadfunktion zurückspringt.
Das funktioniert auch noch. Starte ich allerdings den Thread, lasse ihn laufen bis er sich beendet hat und starte ihn ein weiteres Mal, dann werden die Funktionen wie erwartet ausgeführt. Beim Beenden des Programms allerdings werden Memory-leaks angezeigt.

Detected memory leaks!
Dumping objects ->
thrdcore.cpp(311) : {1009} client block at 0x00827028, subtype c0, 68 bytes long.
a CWinThread object at $00827028, 68 bytes long
thrdcore.cpp(311) : {532} client block at 0x00826FA8, subtype c0, 68 bytes long.
a CWinThread object at $00826FA8, 68 bytes long
Object dump complete.

Wo kommen die her? Beim Beenden des Dialogfensters wird doch das CWinThread-Objekt (ist Member des Dialogs, m_bAutodelete ist auf false gesetzt) bereits entsorgt? Da müsste ja schon beim Schließen des Dialogs was zu sehen sein. Die OnOk und die On Cancel enthalten folgenden Code:

void CKalibrierungWMA::OnOK()
{
	// TODO: Fügen Sie hier Ihren spezialisierten Code ein, und/oder rufen Sie die Basisklasse auf.

	m_bWantExit=true;
	if (m_bIsRunning==true)
	{
		if (m_pThread)
		{
			::WaitForSingleObject(m_pThread->m_hThread,INFINITE);
			delete m_pThread;
			m_bIsRunning=false;
		}
	}
	CDialog::OnOK();
}

Martin Richter

Hast Du mal den trivialen Test im Debugger gemacht, dass dieser delete Code überhaupt ausgeführt wird?

AndyDD

Martin Richter schrieb:

Hast Du mal den trivialen Test im Debugger gemacht, dass dieser delete Code überhaupt ausgeführt wird?

Ja das wird er. Starte ich den Dialog ohne den Thread zu starten, dann wirds übergangen. Starte ich den Dailog und dann den Thread einmal, so funktioniert es. Starte ich den Thread mehrfach (d.h. starten, warten bis er terminiert und wieder starten) dann entsteht das besagte Phänomen.

Martin Richter

Dann wird eben unter bestimmten Umständen, der Destruktoir nicht mehr aufgerufen.
Startets Du etwa den Thread mehrfach in dem Dialog? Du hast nur einen Zeiger?

AndyDD

Martin Richter schrieb:

Dann wird eben unter bestimmten Umständen, der Destruktoir nicht mehr aufgerufen.
Startets Du etwa den Thread mehrfach in dem Dialog? Du hast nur einen Zeiger?

Meinst Du den Destruktor vom Dialog? Doch, der wird aufgerufen. Hab da ein TRACE drin und das wird auch angezeigt.
Der Thread wird schon mehrfach gestartet, jedoch läuft er nicht parallel. Im Dialog gibts einen Button. Wird er gedrückt dann wird der Thread gestartet und dieser Button deaktiviert. Der Button ist die einzige Möglichkeit den Thread zu starten. Terminiert der Thread, so wird der Button wieder aktiv. Diese Aktivierung mach ich in der Threadfunktion vor dem return-Aufruf. Weiß nicht ob das so elegant ist, hatte keine andere Lösung. Den Rückgabewert der AfxBeginThread speichere ich in eine Membervariable des Dialogs. Klar, hier wird immer wieder die gleiche Variable verwendet. Könnte es daran liegen? Wenn ich nämlich m_bAutodelete auf true lasse ist das Problem weg.
Komischerweise hab ich ein anderes Dialogfeld und da hab ich die gleiche Struktur. Dort funktioniert es....

Martin Richter

Und was passiert nurn, wenn Du den Button drückst, der Thread startet, beendet und Du startest ihn erneut?

Wer löscht, dann das Thread Objekt.

AndyDD

Martin Richter schrieb:

Und was passiert nurn, wenn Du den Button drückst, der Thread startet, beendet und Du startest ihn erneut?

Wer löscht, dann das Thread Objekt.

Wenn ich den Button drücke passiert folgendes:

void CKalibrierungWMA::OnBnClickedStatisch()
{
	// TODO: Fügen Sie hier Ihren Kontrollbehandlungscode für die Benachrichtigung ein.
	m_bWantExit=false;
	m_pThread=AfxBeginThread(Messung,this,THREAD_PRIORITY_NORMAL,0,0,NULL);
	m_pThread->m_bAutoDelete=false;
	if (m_pThread!=NULL) 
	{
		m_bIsRunning=true;
		GetDlgItem(IDC_STATISCH)->EnableWindow(false);
		GetDlgItem(IDC_ZYLINDER_KLEIN)->EnableWindow(false);
		GetDlgItem(IDC_ZYLINDER_GROSS)->EnableWindow(false);
	}
	else
	{
		AfxMessageBox("Fehler bei Initialisieren des Threads",MB_ICONSTOP | MB_OK);
	}
}

Die Threadfunktion sieht so aus:

UINT CKalibrierungWMA::Messung(LPVOID pParam)
{
	//Zeiger auf das Dialogobjekt holen
	CKalibrierungWMA* pDialog=(CKalibrierungWMA*)pParam;
	ASSERT_VALID(pDialog);

	//hier signalisieren, dass der Thread läuft
	pDialog->m_bIsRunning=true;

	// == Ab hier kritischer Bereich ==
	{
		CSingleLock lock(&pDialog->m_csProtectVar,TRUE);
		//hier laufen Berechnungen
	}

		pDialog->GetDlgItem(IDC_STATISCH)->EnableWindow(true);
		pDialog->GetDlgItem(IDC_ZYLINDER_KLEIN)->EnableWindow(true);
		pDialog->GetDlgItem(IDC_ZYLINDER_GROSS)->EnableWindow(true);
		::PostMessage(pDialog->GetSafeHwnd(),WM_UPDATEWINDOW,0,0);

	return 0;
}

Damit ist sichergestellt, dass der Thread erst wieder gestartet werden kann wenn er terminiert hat. Aufegeräumt wird in der OnCancel bzw. OnOK

void CKalibrierungWMA::OnOK()
{
    // TODO: Fügen Sie hier Ihren spezialisierten Code ein, und/oder rufen Sie die Basisklasse auf.

    m_bWantExit=true;
    if (m_bIsRunning==true)
    {
        if (m_pThread)
        {
            ::WaitForSingleObject(m_pThread->m_hThread,INFINITE);
            delete m_pThread;
            m_bIsRunning=false;
        }
    }
    CDialog::OnOK();
}

Wo liegt hier der Fehler? Wie gesagt ich hab noch einen Dialog nach dem gleichen Prinzip gebaut, da gehts...

CStoll

Und wenn du jetzt zweimal auf "statisch" klickst, werden zwei Threads gestartet - um den ersten kümmert sich niemand mehr, also bleibt er liegen. (wenn du m_bAutoDelete gesetzt lässt, löscht sich der Thread selber, wenn er fertig ist - aber dann kannst du Probleme bekommen, wenn du danach noch auf ihn warten willst)

Martin Richter

Dein Fehler liegt darin:
Dein Thread terminiert. Und in der Threadfunktion am Ende erlaubst Du erneu, dass der Button geklickt werden darf. Und nun entsorgt aber keiner mehr Dein m_pThread Objekt!
Es ist genau, dass was ich schon angemerkt habe!

Was macht WM_UPDATEWINDOW?

Anmerkungen:
1. AfxBeginThread kann NULL returnieren. Dann krachts es bei Dir...
2. Der Thread kann so schnell terminieren, dass der Zeiger ungültig wird, bevor Du m_bAutoDelete auf false setzen kannst.
3. Es ist ganz ganz mieser Stil GUI Elemente aus einem Thread heraus zu manipulieren. Die Messages müssen durch die MessageQueue und dies führt gerne mal zu Deadlocks und sonstigen Problemen. (EnableWindow löst einige Nachricten aus, wenn Dir das nicht klar war).

AndyDD

Martin Richter schrieb:

Dein Fehler liegt darin:
Dein Thread terminiert. Und in der Threadfunktion am Ende erlaubst Du erneu, dass der Button geklickt werden darf. Und nun entsorgt aber keiner mehr Dein m_pThread Objekt!
Es ist genau, dass was ich schon angemerkt habe!

Was macht WM_UPDATEWINDOW?

Anmerkungen:
1. AfxBeginThread kann NULL returnieren. Dann krachts es bei Dir...
2. Der Thread kann so schnell terminieren, dass der Zeiger ungültig wird, bevor Du m_bAutoDelete auf false setzen kannst.
3. Es ist ganz ganz mieser Stil GUI Elemente aus einem Thread heraus zu manipulieren. Die Messages müssen durch die MessageQueue und dies führt gerne mal zu Deadlocks und sonstigen Problemen. (EnableWindow löst einige Nachricten aus, wenn Dir das nicht klar war).

Ich habs befürchtet. Ist das erste größere Projekt mit mehreren Threads und ich bin sicher dabei, die ganzen Anfängerfehler mitzunehmen. Das mit EnableWindow ist mir nicht klar. WM_UPDATEWINDOW bewirkt das hier:

LRESULT CKalibrierungWMA::OnUpdateWindow(WPARAM wParam, LPARAM lParam)
{
	UpdateData(false);
	Invalidate();
	UpdateWindow();
	return LRESULT();
}

@CStoll: genau das hab ich ja ausprobiert. Warten müsste ich ja nur wenn er noch läuft und jemand Cancel oder Ok drückt. Genau das geht ja aber nicht wenn er sich selber löschen soll.

Es treten keine memory leaks auf wenn ich in der OnBnClickedStatisch() des alte Objekt vernichte.

void CKalibrierungWMA::OnBnClickedStatisch()
{
	m_bWantExit=false;
	if (m_pThread)
	{
			::WaitForSingleObject(m_pThread->m_hThread,INFINITE);
			delete m_pThread;
			m_bIsRunning=false;
	}
	m_pThread=NULL;
	m_pThread=AfxBeginThread(Messung,this,THREAD_PRIORITY_NORMAL,0,0,NULL);
	m_pThread->m_bAutoDelete=false;
	if (m_pThread!=NULL) 
	{
		m_bIsRunning=true;
		GetDlgItem(IDC_STATISCH)->EnableWindow(false);
		GetDlgItem(IDC_ZYLINDER_KLEIN)->EnableWindow(false);
		GetDlgItem(IDC_ZYLINDER_GROSS)->EnableWindow(false);
	}
	else
	{
		AfxMessageBox("Fehler bei Initialisieren des Threads",MB_ICONSTOP | MB_OK);
	}
}

@Martin: wie könnte ich die Interaktion mit dem GUI lösen? Eine Message schicken und in der das Enablen/Disablen durchführen? Oder sollte man eine Art Instanzzähler einbauen der prüft ob ein Threadobjekt vorhanden ist oder nicht. Nur wo kann ich im Programm des Threadobjekt vernichten?

CStoll

AndyDD schrieb:

@CStoll: genau das hab ich ja ausprobiert. Warten müsste ich ja nur wenn er noch läuft und jemand Cancel oder Ok drückt. Genau das geht ja aber nicht wenn er sich selber löschen soll.

Genau das meinte ich - wenn der Thread sich selber löscht, kannst du nach seinem Ende nicht mehr auf ihn warten (das Thread-Handle wird ungültig), wenn er sich nicht löscht, mußt du das übernehmen. Du könntest eine eigene Synchronisation aufbauen (z.B. über ein PostMessage(), bei dessen Verarbeitung du den Thread entsorgst) oder du löschst den alten Thread, bevor du einen neuen startest.

@EnableWindow(): Fenster-Handles sind nicht Thread-sicher - und die Verwendung außerhalb des Hauptthreads kann katastrophale Folgen haben (ich hab's schon geschafft, mit sowas mein Programm UND Visual Studio abzuschießen). Überlass die Arbeit an der Oberfläche da lieber dem Hauptthread (die Verarbeitungsroutine der geposteten Nachricht wäre eine Möglichkeit).

AndyDD

CStoll schrieb:

Genau das meinte ich - wenn der Thread sich selber löscht, kannst du nach seinem Ende nicht mehr auf ihn warten (das Thread-Handle wird ungültig), wenn er sich nicht löscht, mußt du das übernehmen. Du könntest eine eigene Synchronisation aufbauen (z.B. über ein PostMessage(), bei dessen Verarbeitung du den Thread entsorgst) oder du löschst den alten Thread, bevor du einen neuen startest.

@EnableWindow(): Fenster-Handles sind nicht Thread-sicher - und die Verwendung außerhalb des Hauptthreads kann katastrophale Folgen haben (ich hab's schon geschafft, mit sowas mein Programm UND Visual Studio abzuschießen). Überlass die Arbeit an der Oberfläche da lieber dem Hauptthread (die Verarbeitungsroutine der geposteten Nachricht wäre eine Möglichkeit).

Ich lösch ja jetzt den alten Thread bevor ich einen neuen starte (siehe letzter Post). Ist das ein Weg den man gehen kann? Was meinst du mit eigener Synchronisation (Postmessage die den Thread entsorgt)? Soll ich vor dem return eine Message abschicken, die dann in der On...Message das delete macht?
Wenn ich den letzten Abschnitt richtig interpretieren, dann soll ich eine weitere Message abschicken, die dann dem Dialog sagt, er solle selbst seine Buttons disablen/enablen...

CStoll

AndyDD schrieb:

Ich lösch ja jetzt den alten Thread bevor ich einen neuen starte (siehe letzter Post). Ist das ein Weg den man gehen kann? Was meinst du mit eigener Synchronisation (Postmessage die den Thread entsorgt)?

Ja, das ist auch ein möglicher Weg.

Soll ich vor dem return eine Message abschicken, die dann in der On...Message das delete macht?
Wenn ich den letzten Abschnitt richtig interpretieren, dann soll ich eine weitere Message abschicken, die dann dem Dialog sagt, er solle selbst seine Buttons disablen/enablen...

Du hast doch schon ein PostMessage() am Ende des Threads - in dem zugehörigen Handler kannst du dann die Buttons wieder reaktivieren. (und in diesem Handler könntest du auch deinen Thread beseitigen)

AndyDD

Ok, ich werde das mal so versuchen. Dort muss ich doch aber auch mit WaitForSingleObject auf das returnieren warten, da ja nicht sicher ist, ob er schon ruturniert hat.
Erklärt aber immer noch nicht warum es bei dem anderen Dialogfeld (scheinbar) funktioniert...

AndyDD

Funktioniert jetzt einwandfrei. Aber warum bekomme ich ein ASSERT an der Stelle, wo ich den Zeiger überprüfe?
Er ist nicht NULL und wen ichs auskommentiere läufts trotzdem.

UINT CKalibrierungWMA::Messung(LPVOID pParam) 
{ 
    //Zeiger auf das Dialogobjekt holen 
    CKalibrierungWMA* pDialog=(CKalibrierungWMA*)pParam; 
    ASSERT_VALID(pDialog); //<-hier gibts immer ein Assert

    //hier signalisieren, dass der Thread läuft 
    pDialog->m_bIsRunning=true; 

    // == Ab hier kritischer Bereich == 
    { 
        CSingleLock lock(&pDialog->m_csProtectVar,TRUE); 
        //hier laufen Berechnungen 
    } 

        pDialog->GetDlgItem(IDC_STATISCH)->EnableWindow(true); 
        pDialog->GetDlgItem(IDC_ZYLINDER_KLEIN)->EnableWindow(true); 
        pDialog->GetDlgItem(IDC_ZYLINDER_GROSS)->EnableWindow(true); 
        ::PostMessage(pDialog->GetSafeHwnd(),WM_UPDATEWINDOW,0,0); 

    return 0; 
}

CStoll

Schau dir doch mal im Debugger an, welche Überprüfung im ASSERT_VALID() da zugeschlagen hat.

AndyDD

CStoll schrieb:

Schau dir doch mal im Debugger an, welche Überprüfung im ASSERT_VALID() da zugeschlagen hat.

In der Meldung steht, dass die Ausnahme in der wincore.cpp, Line 888 stattgefunden hat.

CObject* p;
		ASSERT((p = pMap->LookupPermanent(m_hWnd)) != NULL ||
			(p = pMap->LookupTemporary(m_hWnd)) != NULL); //Line 888
		ASSERT((CWnd*)p == this);   // must be us

		// Note: if either of the above asserts fire and you are
		// writing a multithreaded application, it is likely that
		// you have passed a C++ object from one thread to another
		// and have used that object in a way that was not intended.
		// (only simple inline wrapper functions should be used)
		//
		// In general, CWnd objects should be passed by HWND from
		// one thread to another.  The receiving thread can wrap
		// the HWND with a CWnd object by using CWnd::FromHandle.
		//
		// It is dangerous to pass C++ objects from one thread to
		// another, unless the objects are designed to be used in
		// such a manner.

Bei VERIFY funktionierts.

CStoll

Da steht doch die Erklärung - du hast ein CWnd-Objekt (deinen Dialog) über Thread-Grenzen hinweg übergeben.

Das Problem hatte ich oben schonmal erwähnt - Fenster-Handles sind nur innerhalb ihres eigenen Threads gültig, darum fliegt dir die Überprüfung um die Ohren, wenn du aus einem anderen Thread heraus die Gültigkeit des Handles überprüfen willst.

Gegenfrage: Was willst du mit dem ASSERT_VALID() überhaupt erreichen?

Martin Richter

CStoll schrieb:

Das Problem hatte ich oben schonmal erwähnt - Fenster-Handles sind nur innerhalb ihres eigenen Threads gültig, darum fliegt dir die Überprüfung um die Ohren, wenn du aus einem anderen Thread heraus die Gültigkeit des Handles überprüfen willst.

Das ist nicht korrekt. Ein Fenster-Handle hat prozessweite Gültigkeit. Nur ist ein Fenster threadafin!
Richtig ist das ein CWnd* Zeiger genauzso threadafin ist und nur innerhalb des erzeugenden Threads verwendet werden sollen.

Fenster handle können immer über Threadgrenzen hinweg verwendet werden. Man sollte sich nur klar sein, dass bestimmte UI Funktionen und SendMessage eine Synchronisation der Threads über die Message Loop auslösen. Das führt dann oft zu einem Deadlock...

AndyDD

CStoll schrieb:

Da steht doch die Erklärung - du hast ein CWnd-Objekt (deinen Dialog) über Thread-Grenzen hinweg übergeben.

Das Problem hatte ich oben schonmal erwähnt - Fenster-Handles sind nur innerhalb ihres eigenen Threads gültig, darum fliegt dir die Überprüfung um die Ohren, wenn du aus einem anderen Thread heraus die Gültigkeit des Handles überprüfen willst.

Gegenfrage: Was willst du mit dem ASSERT_VALID() überhaupt erreichen?

In einem anderen Projekt ist es schon mal vorgekommen, dass der Zeiger NULL war. Dann ist ja auch der Zugriff Essig. Das wollte ich prüfen. Ich gebe Dir Recht, dass das zugegeben hier nicht ganz sinnvoll ist, da der Thread aus dem Dialog heraus aufgerufen wird und der Zeiger theoretisch immer gültig sein dürfte.
Ich hab jetzt alle UI-Funktionen da raus genommen, dadurch wird die Struktur auch klarer. Der Thread fragt die Messkarte ab, rechnet und schreibt innerhalb einer critical section was in die Member des Dialogs. Dann kommt die PostMessage zum Neuzeichnen und Aufräumen des Threadobjektes.
Danke Euch beiden nochmals für die Hilfe.