ThreadSafe ...



  • hallo,

    also ich habe eine DLL geschrieben,
    in der ich eine Klasse CriticalSection habe.
    wenn man die DLL nutzt und eine eigene Klasse von dieser ableitet, so soll die abgeleitete Klasse Thread-Safe sein.

    dazu der Code der CriticalSection-Klasse:

    ct_critical.hpp

    #ifndef __CT_CRITICAL_SECTION_HPP__
    #define __CT_CRITICAL_SECTION_HPP__
    
    #include "ct_types.hpp"
    
    #ifdef CTLIB2008_EXPORTS
    #define CS __declspec(dllexport)
    #else
    #define CS __declspec(dllimport)
    #endif
    
    namespace ctlib
    {
    	class CS CriticalSection
    	{
    	protected:
    		ct_pointer pCriticalSection;
    	public:
    		CriticalSection(void);
    		virtual ~CriticalSection(void);
    		virtual void Lock(void);
    		virtual void Unlock(void);
    	};
    	typedef class CriticalSection ThreadSafe;
    };
    
    #endif
    

    critical.cpp

    #include <windows.h>
    #include "ct_critical.hpp"
    
    ctlib::CriticalSection::CriticalSection(void) : pCriticalSection(reinterpret_cast<ctlib::ct_pointer>(new CRITICAL_SECTION))
    {
    	InitializeCriticalSection(reinterpret_cast<LPCRITICAL_SECTION>(pCriticalSection));
    }
    
    ctlib::CriticalSection::~CriticalSection(void)
    {
    	DeleteCriticalSection(reinterpret_cast<LPCRITICAL_SECTION>(pCriticalSection));
    	delete (reinterpret_cast<LPCRITICAL_SECTION>(pCriticalSection));
    }
    
    void ctlib::CriticalSection::Lock(void)
    {
    	while (!TryEnterCriticalSection(reinterpret_cast<LPCRITICAL_SECTION>(pCriticalSection)));
    }
    
    void ctlib::CriticalSection::Unlock(void)
    {
    	LeaveCriticalSection(reinterpret_cast<LPCRITICAL_SECTION>(pCriticalSection));
    }
    

    dann habe ich ein Testprogramm geschrieben:

    #pragma comment(lib, "CTLib2008.lib")
    
    #include <windows.h>
    #include <conio.h>
    #include <cstdio>
    #include <ctlib/ct_critical.hpp>
    
    class CPoint : ctlib::CriticalSection
    {
    protected:
    	int x, y;
    public:
    	CPoint(void) : x(0), y(0) { ctlib::CriticalSection::CriticalSection(); }
    	CPoint(int _x, int _y) : x(_x), y(_y) { ctlib::CriticalSection::CriticalSection(); }
    	virtual ~CPoint(void) { ctlib::CriticalSection::~CriticalSection(); }
    	void SetX(int _x) { Lock(); x = _x; Unlock(); }
    	void SetY(int _y) { Lock(); y = _y; Unlock(); }
    	void SetXY(int _x, int _y) { Lock(); x = _x; y = _y; Unlock(); }
    	int GetX(void) { Lock(); int ret = x; Unlock(); return ret; }
    	int GetY(void) { Lock(); int ret = y; Unlock(); return ret; }
    	void GetXY(int* px, int* py) { Lock(); *px = x; *py = y; Unlock(); }
    };
    
    DWORD WINAPI TestThread(LPVOID pParam)
    {
    	CPoint* pPoint = reinterpret_cast<CPoint*>(pParam);
    	for (int n = 0; n < 1000; n++)
    	{
    		pPoint->SetXY(n, n);
    	}
    	return 0;
    }
    
    int main(int argc, char** argv)
    {
    	CPoint p;
    	DWORD dwID;
    	HANDLE hThread = CreateThread(NULL, 0, TestThread, reinterpret_cast<LPVOID>(&p), CREATE_SUSPENDED, &dwID);
    	if (!hThread)
    	{
    		printf("Fehler: Konnte Thread nicht erstellen!\n");
    		_getch();
    		return -1;
    	}
    	SetThreadPriority(hThread, THREAD_PRIORITY_BELOW_NORMAL);
    	ResumeThread(hThread);
    	for (int n = 0; n < 10; n++)
    	{
    		int x, y;
    		p.GetXY(&x, &y);
    		printf("n=%d\tx=%d\ty=%d\n", n, x, y);
    	}
    	WaitForSingleObject(hThread, INFINITE);
    	_getch();
    	return 0;
    }
    

    das kompiliert und linkt auch einwandfrei.
    aber am Ende der main sagt mein Debugger:

    First-chance exception at 0x7c91eb74 in ctlib_test.exe: 0xC0000008: An invalid handle was specified.

    und als Stelle des Fehlers wurde mir das angezeigt:

    ctlib::CriticalSection::~CriticalSection(void)
    {
    	DeleteCriticalSection(reinterpret_cast<LPCRITICAL_SECTION>(pCriticalSection)); // <-- hier liegt der Fehler
    	delete (reinterpret_cast<LPCRITICAL_SECTION>(pCriticalSection));
    }
    

    leider habe ich keine Erklärung für den Fehler, da ich ja die Critical Section nicht zwischendurch mal eben delete...

    wäre sehr dankbar, wenn mir jemand helfen könnte, diesen Fehler zu beseitigen

    MfG DrakoXP

    PS.: ctlib::ct_pointer ist ein einfaches typedef auf void*, also:

    namespace ctlib
    {
        typedef void* ct_pointer;
    };
    


  • Warum nimmst Du nicht einfach statt

    protected:
            ct_pointer pCriticalSection;
    

    dieses?

    protected:
            CRITICAL_SECTION criticalSection;
    

    Damit sparst Du Dir die new/delete-Aufrufe:

    ctlib::CriticalSection::CriticalSection(void)
    {
        InitializeCriticalSection(&criticalSection);
    }
    


  • weil ich die Klasse abstrahieren wollte...
    soll heißen, man soll das ganze auch mit einem Compiler ohne WinAPI nutzen können,
    also zum Beispiel, jemand hat von VC2005 die Express und kein PSDK...
    dann soll es eben trotzdem gehen,
    und deswegen habe ich eben den void* an der Stelle benutzt



  • Da hast du aber mächtig viele Fehler drinnen.
    Ich hab den Code mal korrigiert, und mit ein paar Kommentaren versehen:
    (Ich hab' mir auch die Freiheit genommen einige "Stilfehler" zu korrigieren 🙂 )

    Header:

    #ifndef INCLUDED_CT_CRITICAL_SECTION_HPP // keine doppelten underscores, solche namen sind reserviert!
    #define INCLUDED_CT_CRITICAL_SECTION_HPP
    
    #include "ct_types.hpp" 
    
    #ifdef CTLIB2008_EXPORTS 
    #define CTLIB2008_EXPORT __declspec(dllexport) // CS ist etwas mager
    #else 
    #define CTLIB2008_EXPORT __declspec(dllimport) 
    #endif 
    
    namespace ctlib 
    { 
    	class CTLIB2008_EXPORT CriticalSection 
    	{ 
    		//protected: // das sollte IMO private sein
    	private:
    		//ct_pointer pCriticalSection; //das geht viel eleganter:
    		class Impl; // forward decl einer nested class
    		Impl* m_impl; // member fangen mit "m_" an
    		// NOTE: auto_ptr<Impl> hier wäre ein fehler, da "Impl" ein "incomplete type" ist!!!
    	public: 
    		CriticalSection(); // das void in den Klammern ist überflüssig
    		virtual ~CriticalSection(); // virtual hier kann sinn machen
    		virtual void Lock(); // virtual hier ist fragwürdig (wollen abgeleitete klassen das überschreiben?)
    		virtual void Unlock(); // ==''==
    
    		// ScopedLock Klasse, damit man nicht mit Lock()/Unlock() rumfummeln muss:
    		class ScopedLock
    		{
    		public:
    			explicit ScopedLock(CriticalSection& cs) :
    				m_cs(cs)
    			{
    				m_cs.Lock();
    			}
    			~ScopedLock()
    			{
    				m_cs.Unlock();
    			}
    		private:
    			CriticalSection& m_cs;
    		};
    
    		// kopieren und zuweisen verhindern
    	private:
    		CriticalSection(CriticalSection const&);
    		CriticalSection& operator =(CriticalSection const&);
    	}; 
    	//typedef class CriticalSection ThreadSafe; // WTF?
    }; 
    
    #endif // INCLUDED_CT_CRITICAL_SECTION_HPP
    

    Cpp:

    #include <windows.h> 
    #include <stdexcept>
    #include "ct_critical.hpp" 
    
    namespace ctlib { // so tun wir uns leichter
    
    // definition nested class "CriticalSection::Impl" kommt hier
    class CriticalSection::Impl
    {
    	friend class ::ctlib::CriticalSection;
    	Impl()
    	{
    		if (!::InitializeCriticalSectionAndSpinCount(&m_cs, 1000)) // InitializeCriticalSectionAndSpinCount hat einen returnwert, und ist daher vorzuziehen
    			throw std::runtime_error("InitializeCriticalSectionAndSpinCount failed.");
    	}
    
    	~Impl()
    	{
    		::DeleteCriticalSection(&m_cs);
    	}
    
    	CRITICAL_SECTION m_cs;
    };
    
    CriticalSection::CriticalSection() : // das ctlib:: brauchen wir nun nichtmehr, und wieder: kein void in den Klammern
    	m_impl(new CriticalSection::Impl())
    { 
        // ::InitializeCriticalSection(...) // brauchen wir jetzt nichtmehr, wird von Impl::Impl erledigt
    } 
    
    CriticalSection::~CriticalSection() // s.o.
    { 
        //::DeleteCriticalSection(static_cast<LPCRITICAL_SECTION>(pCriticalSection)); 
        delete m_impl; // kein cast mehr notwändig
    } 
    
    void CriticalSection::Lock()  // s.o.
    { 
        // while (!TryEnterCriticalSection(reinterpret_cast<LPCRITICAL_SECTION>(pCriticalSection))); // ganz böse, da gibts ne funktion dafür:
        ::EnterCriticalSection(&m_impl->m_cs);
    } 
    
    void CriticalSection::Unlock()  // s.o.
    { 
    	// LeaveCriticalSection(reinterpret_cast<LPCRITICAL_SECTION>(pCriticalSection)); // geht jetzt auch ohne cast
    	::LeaveCriticalSection(&m_impl->m_cs);
    } 
    
    } // namespace ctlib
    

    Testprogramm:

    #pragma comment(lib, "CTLib2008.lib") 
    
    #include <windows.h> 
    #include <conio.h> 
    #include <cstdio> 
    #include <ctlib/ct_critical.hpp> 
    
    class CPoint // hier tun wir jetzt nichtmehr private ableiten, Grund siehe unten
    { 
    //protected: // was hast du nur mit protected...?
    private:
    	ctlib::CriticalSection mutable m_mutex; // hier lebt jetzt unsere CriticalSection, und zwar mutable, so können wir Funktionen auch const machen
        int m_x; // member mit m_, eins pro Zeile
        int m_y; 
    
    public: 
        CPoint() : m_x(0), m_y(0)
        {
    		//ctlib::CriticalSection::CriticalSection(); // das ist FALSCH, basisklassen werden immer automatisch konstuiert
    		// davon abgesehen tun wir jetzt nichtmehr ableiten, aber member werden ja auch netterweise automatisch konstruiert
    		// davon abgesehen tut ctlib::CriticalSection::CriticalSection garnicht was du denkst, sondern erzeugt ein objekt vom Typ ctlib::CriticalSection welches sofort wieder zerstört wird!
    	} 
    
        CPoint(int x, int y) : m_x(x), m_y(y)
        {
    		//ctlib::CriticalSection::CriticalSection(); // s.o.
    	} 
    
        virtual ~CPoint()
        {
    		//ctlib::CriticalSection::~CriticalSection(); // das ist wieder FALSCH, basisklassen werden immer automatisch zerstört
    		// davon abgesehen tun wir jetzt nichtmehr ableiten, aber member werden ja auch netterweise automatisch zerstört
    	}
    
        void SetX(int x)
        {
    		ctlib::CriticalSection::ScopedLock lock(m_mutex);
    		m_x = x;
    	} 
        void SetY(int y)
        {
    		ctlib::CriticalSection::ScopedLock lock(m_mutex);
    		m_y = y;
    	} 
        void SetXY(int x, int y)
        {
    		ctlib::CriticalSection::ScopedLock lock(m_mutex);
    		m_x = x;
    		m_y = y;
        } 
    
        int GetX() const
        {
    		ctlib::CriticalSection::ScopedLock lock(m_mutex); // ScopedLock will ne nicht-const Referenz auf die Mutex, das geht
    		// jetzt hier trotzdem die Funktion "const" ist, da ja "mutable" verwendet wird
    		return m_x; // das ist so OK, "lock" muss zerstört werden NACHDEM der Wert von x kopiert wurde
    	} 
    
        int GetY() const
        {
    		ctlib::CriticalSection::ScopedLock lock(m_mutex);
    		return m_y;
    	} 
    
        void GetXY(int* px, int* py) const
        {
    		ctlib::CriticalSection::ScopedLock lock(m_mutex);
    		*px = m_x;
    		*py = m_y;
    	} 
    }; 
    
    DWORD WINAPI TestThread(LPVOID pParam) 
    { 
        CPoint* pPoint = static_cast<CPoint*>(pParam);  // static_cast reicht aus
        for (int n = 0; n < 1000; n++) 
        { 
            pPoint->SetXY(n, n); 
        } 
        return 0; 
    } 
    
    int main(int argc, char** argv) 
    { 
        CPoint p; 
        DWORD dwID; 
        HANDLE hThread = CreateThread(NULL, 0, TestThread, &p, 0, &dwID);  // reinterpret_cast unnötig, CREATE_SUSPENDED unnötig
        if (!hThread) 
        { 
            printf("Fehler: Konnte Thread nicht erstellen!\n"); 
            _getch(); 
            return -1; 
        } 
        SetThreadPriority(hThread, THREAD_PRIORITY_BELOW_NORMAL); 
        //ResumeThread(hThread); // CREATE_SUSPENDED verwenden wir ja jetzt nichtmehr, sonst wäre das natürlich OK so
        for (int n = 0; n < 10; n++) 
        { 
            int x, y; 
            p.GetXY(&x, &y);
            printf("n=%d\tx=%d\ty=%d\n", n, x, y); 
        } 
        WaitForSingleObject(hThread, INFINITE); 
        _getch(); 
        return 0; 
    }
    


  • Ahso, das ist natürlich ein Grund.

    Mein Gefühl sagt mir, dass es etwas mit dem am Anfang von main deklarierten CPoint zu tun hat. Was passiert, wenn Du die Deklaration in die for-Schleife steckst?



  • Nein.
    Der Fehler der zu dem Absturz geführt hat war den Destruktor von CriticalSection manuell aus dem Destruktor von CPoint aufzurufen.
    Das führt dazu dass 2x DeleteCriticalSection aufgerufen wird, was natürlich Blödsinn ist.



  • ich dachte, das müsste so sein, da ich den Konstruktor ja auch im Konstruktor von CPoint manuell aufrufen muss.



  • DrakoXP schrieb:

    ich dachte, das müsste so sein, da ich den Konstruktor ja auch im Konstruktor von CPoint manuell aufrufen muss.

    Nein, den mußtest du gar nicht manuell aufrufen - und so, wie du es gemacht hast, was es unsinnig (in den Ctor'en hattest du eine lokale CS angelegt, die sofort wieder zerstört wurde).



  • Die Fehler sind alle in dem von mir geposteten Code kommentiert.
    Lies es doch wenigstens mal...



  • naja, so wie ich das jetzt sehe, hätte es gereicht, einfach bei CPoint im CTor und im DTor den Aufruf
    der CTor und Dtor der Basis-Klasse wegzulassen.

    warum also so viel Arbeit?

    und die stilistischen Sachen sind ja für den Endnutzer in dem Sinne soweiso egal...

    gut, was private und protected angeht, hast du Recht...
    es ist wohl eher weniger sinnvoll, wenn geerbte Klassen an den CS-Daten rumspielen können.



  • Die anderen Sachen sind zum Grossteil auch sinnvoll, der reinterpret_cast ist z.B. strenggenommen einfach falsch.
    (Ich weiss, ich weiss, es funktioniert, und auf bestimmten Plattformen ist das Verhalten sogar garantiert, aber eben nicht auf allen. Wenn du schon Zeiger rumcasten willst dann mach aus dem Member einen void-Zeiger, und caste mit static_cast anstelle von reinterpret_cast.)
    Die "const" Geschichten solltest du auch ASAP lernen, je später du das lernst desto mehr Code musst du umschreiben ("const correctness" nachziehen ist ein Haufen Arbeit!).
    TryEnterCriticalSection in einer Schleife ist IMO ganz ganz böse.

    Und die anderen Sachen... wenn du nicht willst lass es bleiben. Sind eigentlich alles nur Dinge die dir später die Arbeit erleichtern, aber wenn du nicht willst... zwingen werde ich dich nicht.



  • ok, dann drei Fragen:

    (1) wie funktioniert const correctness?

    (2) was ist an dem TryEnterCriticalSection in der Schleife so verkehrt?
    also was könnte da schief gehen?

    (3) solange man auf 32Bit-System arbeitet macht reinterpret_cast doch nichts anderes,
    als den kompletten Wert bit für bit zu kopieren, ohne den Typ zu beachten.
    also wo liegt das Problem?



  • ad 1) guck in der C++ FAQ lite nach: http://www.parashift.com/c++-faq-lite/const-correctness.html

    ad 2) es macht dir den Speicherbus/Hypertransport total zu, und verbrät mordsmässig unnötig Rechenzeit. Ganz besonders sobald mal 2 oder mehr Threads gleichzeitig in der "try until available" Schleife hängen. EnterCriticalSection legt den Thread nach einer Weile schlafen, also z.B. wenn nach 1000 mal Probieren immer noch nix gegangen ist (das "1000" hier ist übrigens die Zahl die du bei InitializeCriticalSectionAndSpinCount mitgibst). Der schlafende Thread wird dann aufgeweckt sobald die CRITICAL_SECTION wieder "frei" wird. Ist also deutlichst besser.

    ad 3) Was reinterpret_cast macht ist in vielen Fällen "implementation defined". Aber ganz egal ob es mit reinterpret_cast effektiv geht oder nicht, wieso willst du ein Konstrukt wir reinterpret_cast verwenden wenn der Standard uns void-Zeiger und static_cast für sowas zur Verfügung stellt?

    ----

    Du scheinst nach dem Motto zu arbeiten "wieso richtig wenns falsch auch geht" (bzw. "wieso richtig wenns irgendwie auch geht"). Ich arbeite halt nach dem Motto "wieso falsch wenns richtig auch geht".



  • naja, dann fragt man sich aber, warum reinterpret_cast auch zum Standard gehört...
    also muss es doch da auch irgendwelche Festlegungen geben.



  • DrakoXP schrieb:

    naja, dann fragt man sich aber, warum reinterpret_cast auch zum Standard gehört...
    also muss es doch da auch irgendwelche Festlegungen geben.

    Dann lies halt den Standard.

    Ich verwende reinterpret_cast eben wirklich nur wenn es sein muss, bzw. wenn ich genau weiss was ich tue, und es anders (Standardkonform) zu hässlich würde.
    Mir ist auch klar dass die Bereitschaft eines C++ Programmierers Dinge wie reinterpret_cast freizügig einzusetzen indirekt proportional zum Ausmass seiner C++ Kenntnisse ist, aber deswegen muss ich es noch lange nicht gut finden wenn die Kids wie wild rumcasten :^)




Log in to reply