Aplikacja wielowątkowa - czy koncepcja ok?

0

Witam

Napisałem ostatnio taką testową aplikację wielowątkową (dokładnie 2-wątkową :)) i z racji iż zagadnienia związane z pisaniem aplikacji wielowątkowych należą do tych trudniejszych, tak samo "układanie" aplikacji wielowątkowych też nie jest takie "hop-siup" pomyślałem że zasięgnę opinii osób które mogłyby znaleźć jakieś błędy logiczne i ocenią poniższy przykład.

Krótki opis programu. Klasa CSerial jest niejako "opakowaniem" klasy która otwiera port COM, zamyka, wysyła coś na niego itd.
Klasa ta ma za zadanie operacje IO wrzucić do nowego wątka i sama nie powodować zwiechy samej siebie. w main'ie tworzymy obiekt tej klasy CSerial i z dostępnych funkcji mamy np Switch(state). W funkcji tej jest tworzony nowy wątek (jeśli jeszcze nie istnieje), i przekazujemy do niego wskaźnik na siebie (this). Proces wątka (WorkerThread) z kolei przekazany parametr rzutuje nie ma klasę CSerial, tylko na interfejs który ta klasa implementuje (Iswitch). Interfejs ten udostępnia nam tylko i wyłącznie jedną funkcję obiektu CSerial, która bezpośrednio już operuje na porcie szeregowym. Rzutowanie na interfejs a nie na kompletny obiekt CSerial zabezpiecza nas przed wywołaniem w wątku niedozwolonych funkcji. Może mutex w samym WorkerThread jest na wyrost, bo można założyć że nowy wątek można tworzyć tylko wewnątrz klasy CSerial (a nie jak zakomentowano w main'ie przy naciśnięciu 'm'). Tak samo można założyc, że tylko główny wątek może wywoływać funkcję CSerial::Switch aby między ustawianiem w tej funkcji zmiennej m_operation a faktyczną operacją na porcie w wątku nic nie zmieniło tej zmiennej (dlatego też zastosowałem interfejs).
Dla zasymulowania bardzo długich czasów operacji IO dałem opóźnienia w dwóch miejscach.

#include <windows.h>
#include <conio.h>
#include "Comm.h"

DWORD WINAPI WorkerThread( LPVOID lpParam );
HANDLE ghMutex;

const BYTE STATE_ON = 1;
const BYTE STATE_OFF = 0;

typedef enum PENDING_OP
{
	PO_INIT = 0,
	PO_TURNON,
	PO_TURNOFF,
	PO_TEST,

	PO_NUM	//number of pending operations
}PENDING_OP;


//=============================================================================
//		Interface
//=============================================================================
class Iswitch
{
public:
	virtual void _switch_thread( void ) = 0;
};

//=============================================================================
//				CSerial class
//=============================================================================
class CSerial : private Comm, public Iswitch
{
public:
	CSerial()
	{
		hWriteThread = NULL;
		hLocalMutex = NULL;
		m_State = STATE_ON;
	}
	~CSerial();
	int				Switch( BYTE State );
	BYTE			GetState( void ) { return m_State; }

private:
	void			_switch_thread();

	HANDLE			hWriteThread;
	HANDLE			hLocalMutex;
	DWORD			iThreadId;
	PENDING_OP		m_operation;
	BYTE			m_State;
};


//=============================================================================
//			_switch_thread
//=============================================================================
void CSerial::_switch_thread()
{
/*
	//initialize COM port
	if( !SetupString( "COM1: Baud=9600 data=8 parity=N stop=1" ) )
	{
		printf("Setup port error\n");
		return;
	}

	//Open COM port
	if( !open() )
	{
		printf("Opening port error\n");
		return;
	}
*/
	Sleep(2000);

	switch( m_operation )
	{
	case PO_TURNON:
		m_State = STATE_ON;
		printf("TURN ON\n");
	//	write( "1", 1 );	//BASE
		break;
	case PO_TURNOFF:
		m_State = STATE_OFF;
		printf("TURN OFF\n");
	//	write( "0", 1 );	//BASE
		break;
	default:
		break;
	}

	//BASE, close COM port
//	close();
}

//=============================================================================
//				Write
//=============================================================================
int CSerial::Switch( BYTE State )
{
	if( WaitForSingleObject( hWriteThread, 0 ) == WAIT_TIMEOUT )
	{
		printf("Thread running\n");
		return -1;
	}
	//create local (CSerial class member) mutex to secure m_operation variable ??
	//hLocalMutex = CreateMutex( NULL, TRUE, NULL );

	if( State == STATE_ON )
		m_operation = PO_TURNON;
	else
		m_operation = PO_TURNOFF;

	iThreadId = 1;

	printf("Starting new Thread...\n");
	hWriteThread = CreateThread( NULL, 0, (LPTHREAD_START_ROUTINE)WorkerThread, this, 0, &iThreadId );
	//release local (CSerial class member) mutex
	//ReleaseMutex( hLocalMutex );
	return m_State;
}


//=============================================================================
//				WorkerThread
//=============================================================================
DWORD WINAPI WorkerThread( LPVOID lpParam )
{
	DWORD dwResult = WaitForSingleObject( ghMutex, 0 );
	if( dwResult == WAIT_TIMEOUT )
	{
		printf("MUTEX LOCKED\n");
		return -1;
	}

	ghMutex = CreateMutex( NULL, TRUE, NULL );
	printf( "\nBegin thread...\n" );

	Iswitch *ptr = (Iswitch*)lpParam;

	ptr->_switch_thread();

	Sleep( 3000 );
	printf( "\n\nExiting thread...\n" );
	ReleaseMutex( ghMutex );
	return 0;
}



int main( int argc, char *argv[] )
{
	int key;
	CSerial *port = new CSerial();

	while(1)
	{
		key = _getch();
		if(key=='a')
			printf( "\nres0 = %d\t", port->Switch( STATE_ON ) );
		if(key=='s')
			printf( "\nres0 = %d\t", port->Switch( STATE_OFF ) );
		if(key=='x')
			break;
		if(key=='.')
			printf(".%d.", port->GetState() );
		//if(key=='m')
		//	CreateThread( NULL, 0, (LPTHREAD_START_ROUTINE)WriteThread, &params, 0, &i );

	}

	return 0;
} 
0

Nie wnikałem zbytnio w problem, ale nie rozumiem sensu użycia interfejsu Iswitch. Piszesz, że ten interfejs:

zabezpiecza nas przed wywołaniem w wątku niedozwolonych funkcji.

Tyle tylko, że wątek, o którym mowa, należy do klasy CSerial, zatem ryzyko użycia niedozwolonych metod jest żadne, chyba że sam coś spieprzysz. Można zrobić tak:

class CSerial : private Comm
{
    ...
	
private:
    static DWORD WINAPI WorkerThread(LPVOID);
    void _switch_thread();
	
    ...
};

...

DWORD WINAPI CSerial::WorkerThread( LPVOID lpParam )
{
    ...
    CSerial *ptr = (CSerial*)lpParam;
    ptr->_switch_thread();
    ...
}

I żadnych bezsensownych zabezpieczeń dodawać nie trzeba.

p.s. tworzysz wątki i muteksy, ale nigdzie nie widzę, żebyś je usuwał.

0

Witam i z góry dziękuję za odpowiedź :)

Co do mutexów, to w samej funkcji wątka WorkerThread jest tworzony mutex i tuż przed wyjściem z funkcji jest ReleaseMutex(). Tylko właśnie pisałem że może ten mutex jest na wyrost, ponieważ wcześniej jest sprawdzany czy globalny mutex jest zajęty ( WaitForSingleObject( ghMutex, 0 ); ). Może gdyby WorkerThread był memberem klasy, wtedy obyłoby sie bez globalnego mutexa.

Co do stworzenia funkcji wątka jako membera klasy CSerial, to wtedy funkcja ta musi być statyczna, i można operować tylko na statycznych polach klasy. A przekazując parametr this, w funkcji wątka np chcemy aby ktoś inny mógł korzystać tylko z takich funkcji jakie my mu dostarczymy, dlatego jest interfejs, a nie cała klasa ze wszystkimi bebechami. Nie wiem, może też tak jest bardziej elegancko? :P

A nie chciałbym aby te metody były statyczne, ponieważ co jeśli np będę chciał mieć dwa takie obiekty CSerial gdzie każdy steruje innym portem COM?
Może dobrze by było zrobić konstruktor który jako parametr przyjmuje numer portu COM, i jeśli juz taki został stworzony to zwraca wskaźnik do tego obiektu. Coś a'la taki singleton :)

0

(...) i tuż przed wyjściem z funkcji jest ReleaseMutex().

Tyle że ReleaseMutex nie służy do usuwania muteksa z pamięci, jedynie zmienia jego stan. Muteksy i wątki usuwasz funkcją CloseHandle. Odsyłam do dokumentacji.

(...) i można operować tylko na statycznych polach klasy.

Nie tylko. Po to przekazujesz this przy tworzeniu wątka, żebyś mógł odwoływać się do konkretnej instancji klasy. W swoim przykładzie ów przekazywanie this pominąłem, bo to już masz w swoim kodzie (myślałem, że się domyślisz).

Może dobrze by było zrobić konstruktor który jako parametr przyjmuje numer portu COM (...)

Po to właśnie wymyślono konstruktory parametryczne.

0

A tak racja. Nie wstawiłem CloseHandle zarówno dla globalnego mutexa ghMutex jak i dla uchwytu dla wątka. Co prawda działać działa, ale nie o to tylko chodzi.

Co do przekazywania tego this'a do wątka, to tak, mając go w wątku mogę wywołać każdą publiczną funkcję tej klasy, niekoniecznie statyczną. Oj no chodziło mi tylko o to, czy opakowanie tego w interfejs nie jest jedną z tych rzeczy z typu "good practice". :)

Natomiast o tym konstruktorze parametrycznym i coś na wzór singletona wspomniałem aby zasygnalizować że może istnieć potrzeba posiadania więcej niż jednego obiektu, dlatego może posiadanie jakichś statycznych metod czy pól może być niewskazane. Chociaż z drugiej strony - nie :P
Faktycznie może lepszym sposobem będzie wpakowanie WorkerThread w klasę, a z racji że mamy tam this'a nie martwimy się że jest statyczna. Na dodatek znika nam ten brzydki globalny mutex :) Wszystko jest zamknięte w klasie.

Wrzucę jeszcze kod jakby sie komuś chciało sprawdzić, albo był ciekawy.
Dziękuję za cenne wskazówki, myślę że teraz wygląda to lepiej.

//=============================================================================
//				CSerial class
//=============================================================================
class CSerial : private Comm, public Iswitch
{
public:
	CSerial()
	{
		hWriteThread = NULL;
		hLocalMutex = NULL;
		m_State = STATE_ON;
	}
	~CSerial()
	{
		CloseHandle( hLocalMutex );
		CloseHandle( hWriteThread );
	}

	int						Switch( BYTE State );
	BYTE					GetState( void ) { return m_State; }

private:
	void					_switch_thread();
	static DWORD WINAPI		WorkerThread( LPVOID lpParam );
	HANDLE					hWriteThread;
	HANDLE					hLocalMutex;
	DWORD					iThreadId;
	PENDING_OP				m_operation;
	BYTE					m_State;
	int						m_iValue;
	void					Lock( void )	{ hLocalMutex = CreateMutex( NULL, TRUE, NULL ); }
	BOOL					Unlock( void )	{ return ReleaseMutex( hLocalMutex ); }
};


//=============================================================================
//			_switch_thread
//=============================================================================
void CSerial::_switch_thread()
{
	Sleep(2000);

	switch( m_operation )
	{
	case PO_TURNON:
		m_State = STATE_ON;
		printf("TURN ON\n");
		break;
	case PO_TURNOFF:
		m_State = STATE_OFF;
		printf("TURN OFF\n");
		break;
	default:
		break;
	}
}

//=============================================================================
//				Write
//=============================================================================
int CSerial::Switch( BYTE State )
{
	if( WaitForSingleObject( hLocalMutex, 0 ) == WAIT_TIMEOUT )
	{
		printf("Thread running\n");
		return -1;
	}

	if( State == STATE_ON )
		m_operation = PO_TURNON;
	else
		m_operation = PO_TURNOFF;

	iThreadId = 1;

	printf("Starting new Thread...\n");
	hWriteThread = CreateThread( NULL, 0, (LPTHREAD_START_ROUTINE)CSerial::WorkerThread, this, 0, &iThreadId );
	return m_State;
}


//=============================================================================
//				WorkerThread
//=============================================================================
DWORD WINAPI CSerial::WorkerThread( LPVOID lpParam )
{
	DWORD dwResult = WaitForSingleObject( ghMutex, 0 );
	if( dwResult == WAIT_TIMEOUT )
	{
		printf("MUTEX LOCKED\n");
		return -1;
	}

	printf( "\nBegin thread id %d...\n", GetCurrentThreadId() );

	CSerial *ptr = (CSerial*)lpParam;
	ptr->Lock();	//create mutex
	ptr->_switch_thread();

	Sleep( 3000 );
	printf( "\n\nExiting thread...\n" );
	ptr->Unlock();	//release mutex
	return 0;
}



int main( int argc, char *argv[] )
{
	int key;
	DWORD i = 1;
	CSerial *port = new CSerial();
	CSerial *port2 = new CSerial();

	while(1)
	{
		key = _getch();
		if(key=='a')
		{
			printf( "\nres0 = %d\t", port->Switch( STATE_ON ) );
			printf( "\nres0 = %d\t", port2->Switch( STATE_OFF ) );
		}
		if(key=='s')
			printf( "\nres0 = %d\t", port->Switch( STATE_OFF ) );
		if(key=='x')
			break;
		if(key=='.')
			printf(".%d.", port->GetState() );
	}

	return 0;
}
0
class CSerial : private Comm, public Iswitch //<--- wtf?! Po jakiego grzyba ten Iswitch?
{
public:
	~CSerial()
	{
		CloseHandle( hLocalMutex );
		CloseHandle( hWriteThread );
	}

	...

private:

	...

	void Lock( void )        { hLocalMutex = CreateMutex( NULL, TRUE, NULL ); }
	BOOL Unlock( void )        { return ReleaseMutex( hLocalMutex ); }
};



int CSerial::Switch( BYTE State )
{
	...
	hWriteThread = CreateThread( NULL, 0, (LPTHREAD_START_ROUTINE)CSerial::WorkerThread, this, 0, &iThreadId );
	...
}

Wstawienie CloseHandle w destruktor nie rozwiązuje problemu, bo poprzez wielokrotne wywołanie metod Switch i Lock tworzysz nowe uchwyty gubiąc stare. W destruktorze jedynie zwalniasz uchwyty ostatnio stworzone. Widać, że nie kumasz o co chodzi z muteksami i synchronizacją w ogóle. To powinno wyglądać mniej więcej tak:

class CSerial : private Comm
{
public:
	CSerial()
	{
		hWriteThread = NULL;
		hLocalMutex = CreateMutex( NULL, FALSE, NULL ); //<--- raz tworzymy...
		...
	}

	~CSerial()
	{
		WaitForSingleObject( hWriteThread , INFINITE );
		CloseHandle( hWriteThread );
		CloseHandle( hLocalMutex ); // <--- ... i raz usuwamy
	}

	...
};


DWORD WINAPI CSerial::WorkerThread( LPVOID lpParam )
{
	...
	
	WaitForSingleObject( hLocalMutex, INFINITE ); 
	
	ptr->_switch_thread();
	Sleep( 3000 );
	printf( "\n\nExiting thread...\n" );
	
	ReleaseMutex(hLocalMutex);
	return 0;
}
 



int CSerial::Switch( BYTE State )
{
	if( WaitForSingleObject( hLocalMutex, 0 ) == WAIT_TIMEOUT )
	{
		printf("Thread running\n");
		return -1;
	}

	/* 
	    jeśli wykonanie programu zajdzie do tego miejsca, znaczy, że muteks został przejęty przez wątek 
	    i MUSI być później zwolniony (przestawiony w stan signaled) funkcją ReleaseMutex 
	*/
	...
	
	WaitForSingleObject( hWriteThread , INFINITE ); //<--- czekamy, aż się wątek definitywnie zakończy
	CloseHandle( hWriteThread ); //<--- usuwamy go
	hWriteThread = CreateThread( NULL, 0, (LPTHREAD_START_ROUTINE)CSerial::WorkerThread, this, 0, &iThreadId );
	ReleaseMutex(hLocalMutex); 
	return m_State;
}

Dałem to tylko jako przykład, bo tak naprawdę przy takim trybie pracy wątka muteks nie jest potrzebny. Rolę muteksa w tym przypadku może przejąć sam wątek:

int CSerial::Switch( BYTE State )
{
	if( WaitForSingleObject( hWriteThread, 0 ) == WAIT_TIMEOUT )
	{
		printf("Thread running\n");
		return -1;
	}
	
	...

	CloseHandle( hWriteThread ); 
	hWriteThread = CreateThread( NULL, 0, (LPTHREAD_START_ROUTINE)CSerial::WorkerThread, this, 0, &iThreadId );
	return m_State;
}
0

Nie no fakt, że pisanie aplikacji wielowątkowych pod winapi nie jest takie proste jak np w Javie, ew w nadchodzącym C++11.
Ale dobra, z tymi mutexami zamieszałem. Bo wcześniej chciałem tym ochronić gdyby ktoś wymyślił sobie stworzenie wątka poza klasą CSerial. Dlatego dodałem tego mutexa globalnego. Ale skoro później całość umieściłem w klasie, to faktycznie ten mutex już nie jest potrzebny i wystarczy samo sprawdzenie w metodzie Switch co jeszcze bardziej upraszcza sprawę.

Dzięki za pomoc :)

1 użytkowników online, w tym zalogowanych: 0, gości: 1