Bug in Windows (10) gefunden?

[Bonita.M]

[code="cs"]

hustbaer schrieb:

BTW:
Das...

m_dwOwningThreadId   = 0;
    dwSavedRecusionCount = m_dwRecursionCount;
    m_dwRecursionCount   = 0;

ist einfach nur ein Fehler.

Nein, da ist kein Fehler. Du hast es nur nicht verstanden.
In dem Moment in dem ich das tue bin habe ich noch die Datenstruktur gelockt, und Owners ist > Waiters. Daher darf ich zu diesem Zeitpunkt noch m_dwOwningThread und m_dwRecursionCount verändern. Wenn ich dann aber Waiters inkrementiere, dann ist Owners möglicherweise sogar gleich Waiters, und niemand hat die Struktur gelockt, und alle die auf eine Signalisierung warten warten auch auf den Eintritt in den kritischen Abschnitt (deswegen auch die zwei Strukturen, das Semaphor für die Signalisierung, und das Event für den Eintritt in den kritischen Abschnitt).

hustbaer schrieb:

Wenn SetEvent fehlschlägt kannst du das hier nicht sinnvoll behandeln. Ein assert() ist OK, aber der folgende Retry ist es NICHT.

Das ist konzeptuell nicht anders möglich. Das darf *nie* fehlschlagen, sonst ist eine derart simple Implementation nicht möglich. CRITICAL_SECTION wirft undokumentiererweise wenn das fehlschlägt eine SEH-Exception, und das Programm terminiert. Das ist auch eine saubere Lösung.
Eigentlich kostet die mit SetEvent ausgelöste Wahl eines anderen Threads in die Runnable Liste sicher nur minimal Resourcen. Aber wenn die Resourcen so knapp sind, dass auch das fehlschlägt, dann läuft sicher auch nicht mehr der Debugger. Insofern ist das assert eigentlich Quatsch.

[hustbaer]

Bonita.M schrieb:

hustbaer schrieb:

BTW:
Das...

m_dwOwningThreadId   = 0;
    dwSavedRecusionCount = m_dwRecursionCount;
    m_dwRecursionCount   = 0;

ist einfach nur ein Fehler.

Nein, da ist kein Fehler. Du hast es nur nicht verstanden.

Lol
Nein.
Du hast es nicht verstanden.
Wait aufzurufen während man die Mutex doppelt gelockt hat ist ein Anwendungsfehler.
Deine Implementierung erlaubt das explizit, und gibt in dem Fall sämtliche rekursiven Locks frei. Und genau das ist der Fehler.

Nebenbei bemerkt: Dein Unit-Test testet die Wait-Funktion nicht effektiv. Wenn ich den laufen lasse wird Wait nie aufgerufen. Ich musste erst noch ein Sleep(1) vor dem ::cv.Enter(); in ThreadFunc einfügen damit das if (::dqtr.empty()) jemals true wird.

ps: Natürlich ist das dein Code, und du kannst definieren was du willst. Du kannst also auch definieren dass dein cv::Wait dieses verhalten hat. Dann ist es kein Fehler. Dann ist nur deine cv Klasse ein nutzloses Teil von dem man besser die Finger lassen sollte.

Bonita.M schrieb:

hustbaer schrieb:

Wenn SetEvent fehlschlägt kannst du das hier nicht sinnvoll behandeln. Ein assert() ist OK, aber der folgende Retry ist es NICHT.

Das ist konzeptuell nicht anders möglich. Das darf *nie* fehlschlagen, sonst ist eine derart simple Implementation nicht möglich. CRITICAL_SECTION wirft undokumentiererweise wenn das fehlschlägt eine SEH-Exception, und das Programm terminiert. Das ist auch eine saubere Lösung.
Eigentlich kostet die mit SetEvent ausgelöste Wahl eines anderen Threads in die Runnable Liste sicher nur minimal Resourcen. Aber wenn die Resourcen so knapp sind, dass auch das fehlschlägt, dann läuft sicher auch nicht mehr der Debugger. Insofern ist das assert eigentlich Quatsch.

Nein, das assert ist überhaupt nicht Quatsch.
Quatsch ist mMn. die Annahme dass der häufigste Fehlerfall bei SetEvent "zu wenig Resourcen" wäre. Ich wüsste nicht dass das überhaupt passieren kann. Wieso sollte SetEvent irgendwelche Resourcen benötigen?

Real mögliche Fehlerfälle sind aber 1) das übergebene Handle ist ungültig und 2) das übergebene Handle nat nicht die nötigen Berechtigungen.
Speziell (1) kann passieren wenn der Code der deine Mutex/CV Implementierung verwendet Mist baut (bzw. natürlich auch wenn dein Code Fehler enthält). Handle-Werte werden unter Windows recht schnell recycled. D.h. wenn der Anwendercode einen Fehler dieser Art enthält

HANDLE h = ...
CloseHandle(h);

g_something = new cv();

CloseHandle(h); // <-- Anwenderfehler

Dann kann es sein dass das 2. (falsche) CloseHandle deinen Event freigibt. In dem Fall unendlich zu loopen ist Quatsch. Da gehört ein assert+abort hin.

Bzw. natürlich genau so wenn der User das cv Objekt bereits zerstört hat.

Solche Fälle kann man nicht "korrekt" behandeln. Aber man kann helfen die Fehler schneller zu finden. Das assert macht also durchaus Sinn. Bzw. wenn du sicherstellen willst dass ein Programm welches so einen Fehler enthält nicht unkontrolliert weiterläuft, dann wie gesagt ein abort().

Ohne den Error-Code zu checken einfach zu loopen ist aber haarsträubender Quatsch.

[dot]

@hustbaer: du verschwendest hier deine Zeit...

[Bonita.M]

hustbaer schrieb:

Lol
Nein.
Du hast es nicht verstanden.
Wait aufzurufen während man die Mutex doppelt gelockt hat ist ein Anwendungsfehler.

Erstens ist kein doppeltes Locken möglich; es gibt immer nur einen Owner. Und zweitens hat der Thread bis zum InterlockedAdd den Besitz an dem Mutex, und kann bis dahin m_dwRecursionCount und m_dwOwningId ändern.

... Dann ist nur deine cv Klasse ein nutzloses Teil von dem man besser die Finger lassen sollte.

Der Code is 100% fehlerfrei. Ich habe ihn mit einer Queue die von einem Haupt-Thread 1.000 Worker-Threads füttert und einer Queue die von den 1.000 Worker-Threads wieder mit den Ergebnissen an den Hauptthread am laufen gehabt.

Bonita.M schrieb:

Nein, das assert ist überhaupt nicht Quatsch.
Quatsch ist mMn. die Annahme dass der häufigste Fehlerfall bei SetEvent "zu wenig Resourcen" wäre. Ich wüsste nicht dass das überhaupt passieren kann. Wieso sollte SetEvent irgendwelche Resourcen benötigen?

Du spekulierst hier wild rum. Du weißt aber nicht was SetEvent im Kernel alles lostritt.

Real mögliche Fehlerfälle sind aber 1) das übergebene Handle ist ungültig und 2) das übergebene Handle nat nicht die nötigen Berechtigungen.

Kommt bei mir nicht vor.

[Martin Richter]

Bonita.M schrieb:

Martin Richter schrieb:

Die Windows API hat 0 Zeilen...

Das was ich mit der Windows-API mache ist trivial.

Ich meine die Condition Variable, die in der Win-API integriert ist. Warum selber bauen?

[Bonita.M]

Martin Richter schrieb:

Ich meine die Condition Variable, die in der Win-API integriert ist. Warum selber bauen?

Weil es spaß macht und meine ein gutes Stück effizienter ist (z.B. macht die keine Spurious Wakeups).
Außerdem gibt es ja noch die Möglichkeit, dass jemand Code schreibt der vor Windows Vista bzw. Windows Server 2008 laufen muss; die haben nämlich keine Condition Variable.

[hustbaer]

Bonita.M schrieb:

hustbaer schrieb:

Lol
Nein.
Du hast es nicht verstanden.
Wait aufzurufen während man die Mutex doppelt gelockt hat ist ein Anwendungsfehler.

Erstens ist kein doppeltes Locken möglich; es gibt immer nur einen Owner.

Natürlich ist es das, dafür hast du ja extra m_dwRecursionCount eingebaut. Verstehst du wirklich nicht was ich schreibe oder willst du mich bloss verarschen?

Bonita.M schrieb:

Und zweitens hat der Thread bis zum InterlockedAdd den Besitz an dem Mutex, und kann bis dahin m_dwRecursionCount und m_dwOwningId ändern.

Ja, Mädchen. Ich versteh' dich schon. Du mich bloss nicht. Steig mal von deinem hohen Ross runter und lies Beiträge von anderen mal unter der Annahme dass sie vielleicht a) nicht blöd sind und b) Recht haben könnten.
Dashier geht mit deinem Code:

cs.Enter();
cs.Enter();
cs.Wait();
cs.Leave();
cs.Leave();

Während des Wait() kann die CV dabei von anderen Threads gelockt werden. UND DAS IST EIN FEHLER!

Bonita.M schrieb:

... Dann ist nur deine cv Klasse ein nutzloses Teil von dem man besser die Finger lassen sollte.

Der Code is 100% fehlerfrei. Ich habe ihn mit einer Queue die von einem Haupt-Thread 1.000 Worker-Threads füttert und einer Queue die von den 1.000 Worker-Threads wieder mit den Ergebnissen an den Hauptthread am laufen gehabt.

Kleine, du hast genau keinen Plan von der Praxis. So ein Test besagt nichts.

Bonita.M schrieb:

Nein, das assert ist überhaupt nicht Quatsch.
Quatsch ist mMn. die Annahme dass der häufigste Fehlerfall bei SetEvent "zu wenig Resourcen" wäre. Ich wüsste nicht dass das überhaupt passieren kann. Wieso sollte SetEvent irgendwelche Resourcen benötigen?

Du spekulierst hier wild rum. Du weißt aber nicht was SetEvent im Kernel alles lostritt.

Und? Du genau so wenig. Im Gegensatz zu mir hast du aber Code geschrieben der sehr starke Annahmen darüber macht was mögliche Fehlerfälle bei SetEvent sein könnten.

Bonita.M schrieb:

Real mögliche Fehlerfälle sind aber 1) das übergebene Handle ist ungültig und 2) das übergebene Handle nat nicht die nötigen Berechtigungen.

Kommt bei mir nicht vor.

Lol.
Du hast nichts verstanden.

Danke und tschüss.

[Bonita.M]

[quote]Natürlich ist es das, dafür hast du ja extra m_dwRecursionCount eingebaut. Verstehst du wirklich nicht was ich schreibe oder willst du mich bloss verarschen?[quote]

Wenn auf eine Rekursion getoffen wird wird aber nich gelockt, sondern nur der Rekursionszähler erhöht

Ja, Mädchen. Ich versteh' dich schon. Du mich bloss nicht. Steig mal von deinem hohen Ross runter und lies Beiträge von anderen mal unter der Annahme dass sie vielleicht a) nicht blöd sind und b) Recht haben könnten.
Dashier geht mit deinem Code:

Nein, Du verstehst meinen Code nicht.
Der US-Professor an den ich den geschickt habe hat den aber verstanden.

cs.Enter();
cs.Enter();
cs.Wait();
cs.Leave();
cs.Leave();

Während des Wait() kann die CV dabei von anderen Threads gelockt werden. UND DAS IST EIN FEHLER!

Nein, kann nicht, weil beim wait Owners > waiters ist. Ist Owner - Waiters == 1 gibt es genau einen Thread der das Lock gelockt hat, ist es > 1, gibt es einen Thread der das Lock gelockt hat und zusätzlich Owners - Waiters Threads die auf das Entriegeln warten.

[quote="Bonita.M"]Und? Du genau so wenig. Im Gegensatz zu mir hast du aber Code geschrieben der sehr starke Annahmen darüber macht was mögliche Fehlerfälle bei SetEvent sein könnten.[quote]

SetEvent gibt einen Fehlerode zurück, und ich gehe damit richtig um.

Du hast nichts verstanden.

Du hast nichts verstanden. Meintest Du bist in der Lage Locking-Primitive zu verstehen, kannst es aber offensichtlich nicht.
Wenn keiner den Speicherblock zerschießt in dem mein Lock liegt, dann haben wir es mit einem gültigen Handle zu tun - dann kann SetEvent nur noch aus anderen Gründen fehlschlagen, mglw. weil eben nicht genug Resourcen da sind um einen Thead der nun runnalble wird auf die Runnable-Liste zu packen. Für den Fehlerfall, dass jemand den Speicherblock zerschießt in dem das Handle liegt kann ich nichts.

[Bonita.M]

Natürlich ist es das, dafür hast du ja extra m_dwRecursionCount eingebaut. Verstehst du wirklich nicht was ich schreibe oder willst du mich bloss verarschen?

Wenn auf eine Rekursion getoffen wird wird aber nich gelockt, sondern nur der Rekursionszähler erhöht

Ja, Mädchen. Ich versteh' dich schon. Du mich bloss nicht. Steig mal von deinem hohen Ross runter und lies Beiträge von anderen mal unter der Annahme dass sie vielleicht a) nicht blöd sind und b) Recht haben könnten.
Dashier geht mit deinem Code:

Nein, Du verstehst meinen Code nicht.
Der US-Professor an den ich den geschickt habe hat den aber verstanden.

cs.Enter();
cs.Enter();
cs.Wait();
cs.Leave();
cs.Leave();

Während des Wait() kann die CV dabei von anderen Threads gelockt werden. UND DAS IST EIN FEHLER!

Nein, kann nicht, weil beim wait Owners > waiters ist. Ist Owner - Waiters == 1 gibt es genau einen Thread der das Lock gelockt hat, ist es > 1, gibt es einen Thread der das Lock gelockt hat und zusätzlich Owners - Waiters - 1 Threads die auf das Entriegeln warten. Zugegebenermaßen ist der Begriff Owner hier bissl verkehrt, da nur einer von Owner - Waiters tatsächlich das Lock gelockt hält.

Und? Du genau so wenig. Im Gegensatz zu mir hast du aber Code geschrieben der sehr starke Annahmen darüber macht was mögliche Fehlerfälle bei SetEvent sein könnten.

SetEvent gibt einen Fehlerode zurück, und ich gehe damit richtig um.

Du hast nichts verstanden.

Du hast nichts verstanden. Meintest Du bist in der Lage Locking-Primitive zu verstehen, kannst es aber offensichtlich nicht.
Wenn keiner den Speicherblock zerschießt in dem mein Lock liegt, dann haben wir es mit einem gültigen Handle zu tun - dann kann SetEvent nur noch aus anderen Gründen fehlschlagen, mglw. weil eben nicht genug Resourcen da sind um einen Thead der nun runnable wird auf die Runnable-Liste zu packen. Für den Fehlerfall, dass jemand den Speicherblock zerschießt in dem das Handle liegt kann ich nichts.

[Bonita.M]

löschen

[Mechanics]

Bonita.M schrieb:

Der US-Professor an den ich den geschickt habe hat den aber verstanden.

Das ist nicht beeindruckend. Ich hab den Thread nur überflogen und hab keine Ahnung, von dem eigentlichen Problem, aber ich möchte dich darauf hinweisen, dass Professoren normalerweise keinen besonders guten Ruf haben, bis auf wenige Ausnahmen. Außerdem können wir aus dem Kontext gerissen auch nicht wissen, ob er sich in das Problem überhaupt reingedacht hat oder ob ers nur überflogen hat. Dass er die Idee interessant fand, bedeutet nicht, dass er auch ausschließen kann, dass da Fehler oder potentielle Probleme sind.

[hustbaer]

Du hast immer noch nicht verstanden was ich dir sagen will.
Vielleicht hilft ein Beispiel. Klasse cv wie von dir verlinkt, folgender Test-Code:

#include <cstdio>
#include <cstdlib>
#include <iostream>

DWORD WINAPI ThreadFunc(LPVOID lpvThreadParam);

CondVar cv;

int main()
{
	std::cout << "Locking...\n";
	::cv.Enter();
	std::cout << "Locking again (recursive)...\n";
	::cv.Enter();

	std::cout << "Starting thread...\n";

	HANDLE th = CreateThread(NULL, 0, ThreadFunc, NULL, 0, NULL);

	std::cout << "Waiting...\n";
	::cv.Wait();
	std::cout << "We have been signaled.\n";

	std::cout << "Unlocking...\n";
	::cv.Leave();
	::cv.Leave();

	std::cout << "Bye.\n";

	::WaitForSingleObject(th, INFINITE);
	::CloseHandle(th);
}

DWORD WINAPI ThreadFunc(LPVOID lpvThreadParam)
{
	::cv.Enter();
	std::cout << "WE SHOULD NEVER GET HERE!\n";
	::cv.Release();
	::cv.Leave();

	return 0;
}

Das geht, darf aber nicht gehen.
Klar jetzt?

SetEvent gibt einen Fehlerode zurück, und ich gehe damit richtig um.

Nein, tust du nicht. Ich habe dir auch beschrieben warum. Dass dir das egal ist, bzw. es dir wichtiger zu sein scheint dass du keinen Fehler zugeben musst, OK. Kann mir im Prinzip egal sein. Schreib halt weiter schlechten Code.

[Bonita.M]

Ich hab dich schon verstanden. Im Gegensatz zum vor-vorangegangen Posting hast Du dich mal ausnahmsweise nicht mehrdeutig ausgedrückt.
Natürlich darf das gehen, mein Lock ist ja rekursiv. Wieso soll man auf eine doppelt gelockte CV nicht warten können?
Die inneren Lock- und Leave-Aufrufe machen mit dem Lock garnichts außer den Rekursionszähler zu erhöhen und wieder zu erniedrigen. Wait speichert diesen zwischen, und wenn es ein Signal emfpangen hat, restauriert es den wieder. Gelockt oder entlockt wird also durch die inneren Aufrufe nicht. Das ist absolut kein Bug, dass das geht, sondern so beabsichtigt.

Wenn mein Unit-Test jetzt so aussähe ...

DWORD WINAPI ThreadFunc( LPVOID lpvThreadParam );

struct ThreadResult
{
	DWORD dwThreadId;
	DWORD dwCounter;
};

CondVar            cv;
list<ThreadResult> lstTR;
bool volatile      fStop = false;

int main()
{
	int const              NTHREADS = 16;
	HANDLE                 ahThreads[NTHREADS];
	int                    i;
	std::map<DWORD, DWORD> mTRs;

	for( i = 0; i < NTHREADS; i++ )
		ahThreads[i] = CreateThread( NULL, 0, ThreadFunc, NULL, 0, NULL );

	for( i = 0; i < 10000; i++ )
	{
		ThreadResult tr;

		::cv.Enter();
		::cv.Enter();

		if( ::lstTR.empty() )
			::cv.Wait();

		tr = ::lstTR.front();
		::lstTR.pop_front();
		::cv.Leave();
		::cv.Leave();

		printf( "Thread: %08X - Number: %d\n", (unsigned)tr.dwThreadId, (unsigned)tr.dwCounter );

		if( mTRs.find( tr.dwThreadId ) == mTRs.end() )
			assert(tr.dwCounter == 0),
			mTRs[tr.dwThreadId] = tr.dwCounter;
		else
			assert((mTRs[tr.dwThreadId] + 1) == tr.dwCounter),
			mTRs[tr.dwThreadId] = tr.dwCounter;
	}

	for( ::fStop = true, i = 0; i < NTHREADS; i++ )
		WaitForSingleObject( ahThreads[i], INFINITE );

	return 0;
}

DWORD WINAPI ThreadFunc( LPVOID lpvThreadParam )
{
	ThreadResult tr;

	tr.dwThreadId = GetCurrentThreadId();
	tr.dwCounter  = 0;

	for( ; !::fStop; tr.dwCounter++ )
	{
		::cv.Enter();
		::cv.Enter();
		::lstTR.push_back( tr );
		::cv.Release();
		::cv.Leave();
		::cv.Leave();
		Sleep( 10 );
	}

	return 0;
}

...dann ist das von der Funktionsweise so als würden die inneren Locks und Leaves nicht exisitieren. Der Code läuft genau so wie gewollt durch.

Und die Endlosschleife bei SetEvent ist kein schlecher Code - das geht nicht anders weil die CV so simpel gestrickt ist. Das assert ist eigentlich überflüssig.

[hustbaer]

Achherrjeh, Bonita, ich weiss was rekursive Mutexen sind und wie sie funktionieren. Ich weiss auch was du dir dabei gedacht hast. Ich versuche dir nur zu erklären dass es trotzdem unsinnig ist. Es maskiert gefährliche Anwendungsfehler.

Wenn du es mir nicht glaubst, dann guck doch mal ob du eine Mutex/CV Implementierung findest die so arbeitet. Ich kenne keine. Und das hat schon seinen Grund.

Bonita.M schrieb:

Und die Endlosschleife bei SetEvent ist kein schlecher Code - das geht nicht anders weil die CV so simpel gestrickt ist. Das assert ist eigentlich überflüssig.

mimimi
Ne, ich erklärs dir jetzt nicht nochmal.

[Bonita.M]

hustbaer schrieb:

Achherrjeh, Bonita, ich weiss was rekursive Mutexen sind und wie sie funktionieren. Ich weiss auch was du dir dabei gedacht hast. Ich versuche dir nur zu erklären dass es trotzdem unsinnig ist. Es maskiert gefährliche Anwendungsfehler.

Das doppelte Locken und darin das Wait mag in der Praxis kaum vorkommen, aber es ist kein Anwendungsfehler. Ich finde es nicht falsch diesen seltenen Fall mit zu unterstützen.
Hättest Du dich direkt von Anfang an vollständig und nicht mehrdeutig ausgedrückt, dann hätte ich deine seltsamen Bedenken gleich verstanden. Das hast Du aber nicht.

[hustbaer]

Es ist sehr oft ein Anwendungsfehler. Und wenn es keiner ist, dann ist das rekursive Locken vermeidbar. Es gibt also keinen Grund Wait einen rekursiven Unlock machen zu lassen. Es gäbe dagegen gute Gründe in Wait ein assert() einzubauen um abzusichern dass der Fall eben nicht auftritt.

Aber du weisst ja alles besser.

[hustbaer]

Bonita.M schrieb:

Hättest Du dich direkt von Anfang an vollständig und nicht mehrdeutig ausgedrückt, dann hätte ich deine seltsamen Bedenken gleich verstanden. Das hast Du aber nicht.

Ja, klar, liegt bestimmt an mir.
Muss so sein. Du machst ja keine Fehler. Und verstehst auch alles.

[Bonita.M]

hustbaer schrieb:

Es ist sehr oft ein Anwendungsfehler. Und wenn es keiner ist, dann ist das rekursive Locken vermeidbar. Es gibt also keinen Grund Wait einen rekursiven Unlock machen zu lassen. Es gäbe dagegen gute Gründe in Wait ein assert() einzubauen um abzusichern dass der Fall eben nicht auftritt.

Man kann nicht 100%ig ausschließen, dass jemand sowas absichtlich macht und der Code korrekt ist. Daher ist ein assert() hier falsch.

hustbaer schrieb:

Ja, klar, liegt bestimmt an mir.
Muss so sein. Du machst ja keine Fehler. Und verstehst auch alles.

Ja, das liegt an dir. Z.B. sprachst Du vom doppelten Locken, wobei nicht klar war ob Du damit ein verschachteltes Locken meinst oder ein Locken in zwei Threads. Das ist nicht die einzige Uneindeutigkeit mit der Du kommst. Du vervollständigst deine Aussagen nach und nach, und kommst nicht direkt von Anfang an mit Klarheit.

[Win10BugFinderLober]

Bonita.M schrieb:

löschen

Zuletzt bearbeitet von Bonita.M am 14:45:49 27.05.2016, insgesamt 2-mal bearbeitet

Bonita.M schrieb:

Zuletzt bearbeitet von Bonita.M am 18:11:11 27.05.2016, insgesamt 10-mal bearbeitet

Bonita.M schrieb:

Zuletzt bearbeitet von Bonita.M am 18:23:06 27.05.2016, insgesamt 3-mal bearbeitet

Ich kommentiere das jetzt mal nicht.

[Bonita.M]

Win10BugFinderLober schrieb:

Ich kommentiere das jetzt mal nicht.

Wäre besser wenn mein Vorredner auch mal die Editierfunktion genutzt hätte und sich klar ausgedrückt hätte.