4. Bug in Windows (10) gefunden?

Bug in Windows (10) gefunden?

  • B

    Ich hab für Win32 eine effiziente Condition-Variable entwickelt, hier ist die: http://pastebin.com/QDzAX4fx (ich hab einfach mal alle Header und einen Unit-Test in eine Datei gepackt).
    Mir ist aufgefallen, dass wenn ich in ThreadFunc (im Unit-Test) vor dem ::cv.Leave() ein Sleep( 10000 ) einfüge, die zusätzlich kreierten Threads (die in die ::dqtr schreiben), den Haupt-Thread aushungern lassen, d.h. der bekommt überhauptkeine Rechenzeit mehr. So richtig konnte ich nicht an einen Bug meiner condition-Variable glauben, denn das Design war zu simpel.

    Also habe ich das ganze mal mit einem Event und einem Semaphor zusammengesetzt:

    #include <windows.h>
#include <stdio.h>

HANDLE        hEvt,
              hSema;
bool volatile fReleased = false;

DWORD WINAPI LockAndReleaseThread( LPVOID lpvThreadParam );

int main()
{
    int const NTHREADS = 2;
    HANDLE    ahWait[2];

    ahWait[0] = ::hEvt  = CreateEvent( NULL, FALSE, TRUE, NULL );
    ahWait[1] = ::hSema = CreateSemaphore( NULL, 0, 1, NULL );

    for( int i = 0; i < NTHREADS; i++ )
        CreateThread( NULL, 0, LockAndReleaseThread, NULL, 0, NULL );

    for( ; ; )
    {
        WaitForMultipleObjects( 2, ahWait, TRUE, INFINITE );
        printf( "main thread is holding lock and received signal\n" );
        ::fReleased = false;
        SetEvent( hEvt );
    }

    return 0;
}

DWORD WINAPI LockAndReleaseThread( LPVOID lpvThreadParam )
{
    for( ; ; )
    {
        WaitForSingleObject( hEvt, INFINITE );
        printf( "spawned thread is holding lock\n" );

        if( !::fReleased )
            ReleaseSemaphore( ::hSema, 1, NULL ),
            ::fReleased = true;

        Sleep( 1000 );
        SetEvent( hEvt );
    }

    return 0;
}

    Und siehe da: der LockAndReleaseThread lässt den Haupthread aushungern. Und zwar wenn es zwei oder mehr davon gibt; bei einem geht's noch. Sieht so aus, als würde Windows die Threads die nur auf ein Ereignis warten bevorzugen, und die die auf mehrere warten kommen nicht zum Zuge. Ich finde das ist ein Bug.

    0
  • Mod

    Nö. Das Verhalten ist doch vollkommen logisch.

    1. Deine Threads geben keine Rechenzeit mehr ab, zwischen dem SetEvent und WaitForSingleObject. Zudem ist das Event bereits gesetzt, wenn WaitForSingleObject wieder ausgeführt wird.

    2. Zudem kann in der Sekunde nach dem ReleaseSemaphore nichts passieren, was dem Main Thread hilft, denn der Event ist nicht gesetzt.

    3. Du hast mehrere Threads die auf einen Event warten und einen der auf zwei wartet. Was sagt die Wahrscheinlichkeit? Wenn sich x Threads permanent um den Semaphore prügeln und einer alleine auf die Chance wartet 2 Ereignisse zu bekommen?

    Das Problem liegt in Deinem Aufbau...

    0
  • B

    Martin Richter schrieb:

    1. Deine Threads geben keine Rechenzeit mehr ab, zwischen dem SetEvent und WaitForSingleObject.

    Doch, tun sie. In dem Momeht in dem ein Thead SetEvent sagt guckt der Kernel nach welcher Thread darauf wartet, setzt ihn auf die Runnable-Liste, und das Event ist wieder zurückgesetzt. Das führt dazu, dass der Thread der das Event gesetzt hat keine Rechentzeit bekommt und ein anderer Thread an der Reihe ist; bei zwei Threads spielen die beiden Threads Ping Pong. Der Thread mit dem WaitForMultipleObjects bekommt aber keine ab, obwohl das Semaphor auf 1 ist.

    Ich hab hier nochmal die Funktion LockAndReleaseThread geändert, dass man die wechselden Threads sehen kann:

    char GetID();

DWORD WINAPI LockAndReleaseThread( LPVOID lpvThreadParam )
{
	for( ; ; )
	{
		WaitForSingleObject( hEvt, INFINITE );
		printf( "spawned thread with id %c is holding lock\n", (char)GetID() );

		if( !::fReleased )
			ReleaseSemaphore( ::hSema, 1, NULL ),
			::fReleased = true;

		Sleep( 1000 );
		SetEvent( ::hEvt );
	}

	return 0;
}

char GetID()
{
	static std::unordered_map<DWORD, char> mapTIDsToIDs;
	static char                            nextId = 'A';
	DWORD                                  dwThreadId;

	if( mapTIDsToIDs.find( dwThreadId = GetCurrentThreadId() ) == mapTIDsToIDs.end() )
		return mapTIDsToIDs[dwThreadId] = nextId++;
	else
		return mapTIDsToIDs[dwThreadId];
}

    Kannst ja mal NTHREADS auf 10 setzen, dann siehst Du, dass Threads A bis J sich in alphabetischer Reihenfolge die CPU-Zeit zuschieben.

    Martin Richter schrieb:

    2. Zudem kann in der Sekunde nach dem ReleaseSemaphore nichts passieren, was dem Main Thread hilft, denn der Event ist nicht gesetzt.

    Ach, sag bloß!

    Martin Richter schrieb:

    Zudem ist das Event bereits gesetzt, wenn WaitForSingleObject wieder ausgeführt wird.

    Ja, und das Semaphor auch. Deswegen sollte der Hauptthread irgendwann CPU-Zeit bekommen.
    Bei nur einem zusätzlichen Thread funktioniert die Sache: Hauptthread und zusätzlicher Thread schieben sich gegenseitig die CPU-Zeit zu. Bei mehr als einem zusätzlichen Thread klappt das aber nicht mehr.

    Das Problem liegt in Deinem Aufbau...

    Nein, Du hast den Code einfach nicht verstanden.

    0
  • T

    Hast du beachtet, dass du das Event auf auto-reset gesetzt hast?

    0
  • B

    Techel schrieb:

    Hast du beachtet, dass du das Event auf auto-reset gesetzt hast?

    Ja logisch, sonst würden ja mehrere Threads auf einmal aufgeweckt.

    0
  • C

    WaitForSingleObject() scheint gegenüber WaitForMultipleObjects() im Vorteil zu sein.

    Nimmst Du statt WaitForSingleObject()
    WaitForMultipleObjects(1, &hEvt, TRUE, INFINITE);
    funktionierts wie Du dir das vorstellst. Aber ob man sich auf soetwas überhaupt verlassen sollte...
    Eine condition variable scheint mir hier angebrachter.

    0
  • B

    Caligulaminus schrieb:

    WaitForSingleObject() scheint gegenüber WaitForMultipleObjects() im Vorteil zu sein.

    Nimmst Du statt WaitForSingleObject()
    WaitForMultipleObjects(1, &hEvt, TRUE, INFINITE);
    funktionierts wie Du dir das vorstellst.

    Hab ich jetzt mal gemacht:

    DWORD WINAPI LockAndReleaseThread( LPVOID lpvThreadParam )
{
	for( ; ; )
	{
		WaitForMultipleObjects( 1, &::hEvt, TRUE, INFINITE );
//		WaitForSingleObject( ::hEvt, INFINITE );
		printf( "spawned thread with id %c is holding lock\n", (char)GetID() );

		if( !::fReleased )
			ReleaseSemaphore( ::hSema, 1, NULL ),
			::fReleased = true;

		Sleep( 100 );
		SetEvent( ::hEvt );
	}

	return 0;
}

    Ergebnis ist auf meinem Windows 10 Rechner das selbe.
    Hast Du kleiner Schlingel wohl geraten.

    Caligulaminus schrieb:

    Eine condition variable scheint mir hier angebrachter.

    Es ging mir nicht darum den Code praktisch einzusetzen, sondern nur darum, einen Bug aufzuzeigen. Aber natürlich ist eine Condvar besser, nicht nur weil sie effizienter ist weil sie unter Last i.d.R. nur im Userland arbeitet.

    0
  • B

    Ich habe eine Lösung gefunden.

    So sieht mein neues main() aus:

    int main()
{
	int const NTHREADS = 2;
	HANDLE    ahWait[2];

  	ahWait[0] = ::hEvt  = CreateEvent( NULL, FALSE, TRUE, NULL );
	ahWait[1] = ::hSema = CreateSemaphore( NULL, 0, 1, NULL );
	fReleased = false;

	for( int i = 0; i < NTHREADS; i++ )
		CreateThread( NULL, 0, LockAndReleaseThread, NULL, 0, NULL );

	for( ; ; )
		WaitForSingleObject( ::hSema, INFINITE ),
		WaitForSingleObject( ::hEvt, INFINITE),
//		WaitForMultipleObjects( 2, ahWait, TRUE, INFINITE ),
		printf( "main thread is holding lock and received signal\n" ),
		::fReleased = false,
		SetEvent( hEvt );

    return 0;
}

    Is nen Stück ineffizienter, aber so funktioniert's.
    Würde ich übrigens in umgekehrter Reihenfolge auf das Semaphor und das Event warten, dann gäb es einen Deadlock.

    0
  • Mod

    Jupp. Ich kann das Verhalten sowohl mit Autoreset als auch mit einem Mutex nachvollziehen.
    WaitForMultileObjects scheint "langsamer" zu sein, als WaitForSingleObject...

    Aber ganz verstehe ich Dein Ziel nicht. Ein AutoReset Event ist kein Lock! Denn es bekommt ihn wohl einer genau signalisiert, aber ein zweiter kann genauso schnell kurz danach diesen Event signalisiert bekommen.
    Genau genommen ist die Nutzung des Codes ab if (!::Released) nicht sicher...

    Szenario:

    1. Thread A läuft auf WaitForSingleObject, der Code wird ausgeführt, das Event zurückgesetzt und neu gesetzt.
    2. Thread A läuft auf !::fReleased und führt ReleaseSemaphore aus.
    3. Jetzt kommt (vielleicht) der MainThread an die Reihe und der Semaphore wird wieder gesetzt, das Flag fReleased wird auf false gesetzt.
    4. Nun kommt Thread A wieder an die Reihe kommt aus Release Semaphore zurück und setzt fReleased auf true!

    Ein AutoReset event ist ein Lock. Das Szenario kann ich aktuell natürlich nicht erzeugen, durch den PingPong Effekt.

    Also habe ich Deinen Code umgeschrieben und einen Mutex verwendet, der genau diesen Codeteil um fReleased schützt. Das Verhalten ersacheint mir jetzt auch korrekt.

    #include <windows.h>
#include <unordered_map>
#include <stdio.h> 

HANDLE        hSema, hMtx;
bool volatile fReleased = false;

DWORD WINAPI LockAndReleaseThread(LPVOID lpvThreadParam);

int main()
{
	int const NTHREADS = 4;
	HANDLE    ahWait[2];

	ahWait[0] = ::hSema = CreateSemaphore(NULL, 0, 1, NULL);
	ahWait[1] = ::hMtx = CreateMutex(NULL, FALSE, NULL);

	for (int i = 0; i < NTHREADS; i++)
		HANDLE hThread = CreateThread(NULL, 0, LockAndReleaseThread, NULL, 0, NULL);

	for (;;)
	{
		WaitForMultipleObjects(2, ahWait, TRUE, INFINITE);
		if (::fReleased)
		{
			printf("main thread is holding lock and received signal\n");
			::fReleased = false;
		}
		::ReleaseMutex(hMtx);
	}

	return 0;
}

char GetID();

DWORD WINAPI LockAndReleaseThread(LPVOID lpvThreadParam)
{
	for (;;)
	{
		WaitForSingleObject(hMtx, INFINITE);
		printf("spawned thread with id %c is holding lock\n", (char)GetID());

		if (!::fReleased)
			ReleaseSemaphore(::hSema, 1, NULL),
			::fReleased = true;

		::ReleaseMutex(hMtx);

		Sleep(1000);
	}

	return 0;
}

char GetID()
{
	static std::unordered_map<DWORD, char> mapTIDsToIDs;
	static char                            nextId = 'A';
	DWORD                                  dwThreadId;

	if (mapTIDsToIDs.find(dwThreadId = GetCurrentThreadId()) == mapTIDsToIDs.end())
		return mapTIDsToIDs[dwThreadId] = nextId++;
	else
		return mapTIDsToIDs[dwThreadId];
}
    0
  • Mod

    Upps da warst Du schneller.

    Dein Code ist nicht sicher in Bezug auf fReleased!

    0

  • Nur mal rein prinzipiell: Wieso nicht einfach std::condition_variable verwenden!? Oder die in Windows eingebauten Condition Variables!?

    0
  • B

    Martin Richter schrieb:

    Dein Code ist nicht sicher in Bezug auf fReleased!

    Doch, ist er. Selbst wenn er nicht volatile wär ging das gut weil WaitForSingle/Multiple/Object(s) mindestens acquire- und SetEvent mindestens release-Verhalten hat.
    Das ist schon allein deswegen so weil der Compiler nicht wissen kann ob die genannten beiden Funktionen diese Variable verändern (tun sie nicht, aber das kann der Compiler nicht wissen), und deswegen muss er auf Architekturen wie ARM entsprechende Fences setzen.

    0
  • Mod

    Bonita.M schrieb:

    Martin Richter schrieb:

    Dein Code ist nicht sicher in Bezug auf fReleased!

    Doch, ist er WaiForXxxObjects hat acuire- und release-Verhalten.

    Nicht in Bezug auf WaitFor...

    Schau Dir die Variable fRelease an.

    Diese wird verändert nachdem Der Event getriggert wurde. Aber nichts spricht dagegen, das zwei Threads in kürzester Zeit den Event bekommen und im Falle des Mainthreads einmal das flag auf false und im anderen Fall auf true setzen.

    Folgende Zeitfolge für Thread A und Mainthread M

    A: WaitForMultipleObjects( 1, &::hEvt, TRUE, INFINITE );
    A: if (!::fReleased)
    A: ReleaseSemaphore(::hSema, 1, NULL),
    M: WaitForMultipleObjects(2, ahWait, TRUE, INFINITE);
    M: if (::fReleased)
    A: ::fReleased = true;

    ...

    Das Event is kein Lockmechanismus. Kurz hintereinander können sehr wohl mehrere Threads diesen Wait überlaufen.

    Aber ohne weiteres Wissen was die Worker machen, kann man natürlich nicht sagen, ob das gefährlich ist.

    0
  • B

    löschen!

    0
  • B

    Martin Richter schrieb:

    Schau Dir die Variable fRelease an.

    Nene, der Code ist einwandfrei.
    fRelase wird erstmal nur angefasst wenn man den kritischen Abschnitt mit WairForXxxObject(s) betreten hat. Und gesetzt wird sie von LockAndReleaseThread auch nur wenn er in dem kritischen Abschnitt ist und ::fReleased false ist. Und zurückgesetzt wird er nur vom Hauptthread wenn das Semaphor gesetzt wurde; dan ist nämlich auch ::fReleased true.

    Martin Richter schrieb:

    A: WaitForMultipleObjects( 1, &::hEvt, TRUE, INFINITE );
    A: if (!::fReleased)
    A: ReleaseSemaphore(::hSema, 1, NULL),

    Wieso soll als nächstes M drankommen? Das Event ist doch nicht gesetzt!

    Martin Richter schrieb:

    M: WaitForMultipleObjects(2, ahWait, TRUE, INFINITE);
    M: if (::fReleased)
    A: ::fReleased = true;

    0
  • Mod

    Nein! fRelease ist true NACHDEM ReleaseSemaphore ausgeführt wurde.
    Dazwischen kann alles passieren!

    Nachtrag: Du änderst gerade den Thread auf den ich mich beziehe!

    0
  • B

    Martin Richter schrieb:

    Nein! fRelease ist true NACHDEM ReleaseSemaphore ausgeführt wurde.

    Mann, Mann, Mann, Du hast echt Tomaten auf den Augen.
    Das ist überhauptkein Problem, denn der Hauptthread schafft es erst an ::fReleased heran wenn das Event am Ende von LockAndReleaseThread wieder freigegeben wird.
    Schau dir den Code vor dem nächsten Posting nochmal sorgfältig an!

    0
  • Mod

    Ein normaler und netter Ton wäre OK.

    Deine Funktion heißt LOCK! Es ist kein Lock UND ich habe geschrieben, dass ohne weiteres Wissen, was in diesem Block noch passiert ich mir kein abschließendes Urteil erlauben kann. Aber es ist KEIN LOCK!

    Ansonsten ist für mich die Diskussion beendet. Danke, weitere Kommentare und Antworten sind nicht mehr notwendig.

    0
  • B

    Martin Richter schrieb:

    Deine Funktion heißt LOCK! Es ist kein Lock ...

    Man kann mit einem Autoreset-Event ein Lock realisieren. Das funktioniert dann genauso wie eine CRITICAL_SECTION, nur nicht so effizient (die CRITICIAL_SECTION lockt für den Fall das beide nicht überschneidend locken wollen im Userland durch eine atomare Operation).
    Intern benutzt CRITICAL_SECTION auch ein Event für den Fall, dass überschneidend auf den kritischen Abschnitt zugegriffen wird.
    Daher ist die Bezeichnung *Lock*AndReleaseThread hier voll angebracht.

    Deinen MVP hast Du sicher nicht durch Erkenntnisse im Multithreading bekommen.

    0

  • @Bonita.M

    Sieht so aus, als würde Windows die Threads die nur auf ein Ereignis warten bevorzugen, und die die auf mehrere warten kommen nicht zum Zuge. Ich finde das ist ein Bug.

    Mehr als ein herzhaftes LOL hat das eigentlich nicht verdient.

    0
  • B

    hustbaer schrieb:

    Mehr als ein herzhaftes LOL hat das eigentlich nicht verdient.

    Was intelligentes hast Du wohl nicht zu sagen.

    0
Anmelden zum Antworten