Accessing data from a worker thread

[steg90]

Hi,

I have a thread which accesses a data structure which is held in a different class. The thread modifies this data. The main gui thread then uses this data structure to populate a list. Things are fine for some time but then the application crashes. I'm guessing this is a threading issue?

What is the 'safe' way of accessing 'shared' data from one thread to another?

Kind regards,
Steve

[wysota]

Synchronise access to the data using mutexes (QMutex).

[steg90]

Thanks for that, this is what I've been trying, when each are using the data I put a mutex.lock() and then mutex.unlock() when they finished using the data, still getting a crash. Defo a thread issue, as it is random times, debug window doesn't show me where crash is

If I pass QString's from the thread, then all is fine ( the data structure I mentioned before just had three QString's ). The QString's are created in the worker thread though and I guess in the main thread they will be copies?

Regards,
Steve

[marcel]

Guard it with a QMutex or a QReadWriteLock.
Read about these two Qt classes in Assistant to get an idea about how are they used.

Regards

[wysota]

Hmm... Aren't QStrings thread-safe? Could we see your code?

[steg90]

I guess they are.

This is the function which gets called after the thread has changed the data ( m_can[i] ), the thread emits scrolltable function which ends up calling the code below:

void DATreeModel::setCanData( int nAmount, int nCount )
{
	int row = rowCount();	
 
	beginInsertRows(QModelIndex(), row, row + nAmount - 1 );  // insert nAmount of rows
 
	QMutex mutex;
	mutex.lock();
 
	for( int i = 0; i < nAmount; i++ )
	{
		m_Data.append(m_can[i].strId);
		m_Data.append(m_can[i].strTime);
		m_Data.append(m_can[i].strData);
		QString strCount;
		strCount.sprintf( "%i", nCount );	
		m_Data.append(strCount);
		nCount++;
 
	}
	mutex.unlock();
 
	endInsertRows();
}

void DATreeModel::setCanData( int nAmount, int nCount )
{
	int row = rowCount();	

	beginInsertRows(QModelIndex(), row, row + nAmount - 1 );  // insert nAmount of rows

	QMutex mutex;
	mutex.lock();

	for( int i = 0; i < nAmount; i++ )
	{
		m_Data.append(m_can[i].strId);
		m_Data.append(m_can[i].strTime);
		m_Data.append(m_can[i].strData);
		QString strCount;
		strCount.sprintf( "%i", nCount );	
		m_Data.append(strCount);
		nCount++;
		
	}
	mutex.unlock();

	endInsertRows();
}

To copy to clipboard, switch view to plain text mode 

This is the thread code :

void CanRead::run()
{
	m_nCount = 0;
 
	m_bCancelled = false;
	unsigned long nMsgAmount = 1;
 
	QString szTemp = "";
	unsigned int iCanId = 0;
 
	QString strCanId = "";
	QString strData = "";
 
	QMutex mutex;
 
	while( !m_bCancelled )
	{
#ifdef __CANON_
		unsigned long numMsgs = 1;
		PASSTHRU_MSG *pRxMsg = NULL;
		pRxMsg = theApp->GetMessageDetail( &numMsgs, 0 );
		nMsgAmount = numMsgs;  // store amount of messages we got
 
		strData = "";
 
		int index = 0;
		// Read can data, store in buffer ( read say N amount and then emit? )
		while ( numMsgs && !m_bCancelled )
		{
			mutex.lock();
			szTemp = "";
			m_can[index].strData = "";
			iCanId = 0;
			for ( int iLoop = 0; iLoop < 4; iLoop++)
			{
				iCanId = (iCanId << 8 ) | pRxMsg->Data[iLoop];
			}
 
			QString strId;
			strId.sprintf( "%d", iCanId );
			strCanId = strId;
 
 
			QString strName = theApp->m_dcb.GetMessageName( strId );
			if( strName != "" )
				m_can[index].strId = strName;
			else
			{
				if( pRxMsg->RxStatus & 0x100 )
				{
					m_can[index].strId.sprintf( "%X X", iCanId);
					strCanId.sprintf( "%X X", iCanId );
				}
				else
				{
					m_can[index].strId.sprintf( "%03X", iCanId);
					strCanId.sprintf( "%03X", iCanId );
				}
			}
 
			for ( int iLoop = 4; (unsigned)iLoop < (pRxMsg->DataSize ); iLoop++)
			{
				szTemp.sprintf("%02X ", pRxMsg->Data[iLoop]);
				m_can[index].strData += szTemp;
				strData += szTemp;
			}
 
			m_can[index].strTime.sprintf("%04d", pRxMsg->Timestamp);
 
			numMsgs--;
 
			m_nCount++;
 
			mutex.unlock();
 
		}  // end while ( ulNoMsgs )
 
 
#else  // some dummy data for testing 		
		QString strId = theApp->m_dcb.GetMessageName( "1911" );
		if( strId != "" )
			m_can[0].strId = strId;
		else
			m_can[0].strId = "0xfea";
 
		m_can[0].strData = "222222222222";
		m_can[0].strTime = "----";
 
		m_nCount += nMsgAmount;
 
		strCanId = "1911";
		strData = "2222222222";
#endif		
		// send out signal
		if( nMsgAmount > 0 )
			emit scrolltable( m_nCount - 2, nMsgAmount );
		msleep(10);
 
	} // end while ( true )
 
}

void CanRead::run()
{
	m_nCount = 0;
	
	m_bCancelled = false;
	unsigned long nMsgAmount = 1;

	QString szTemp = "";
	unsigned int iCanId = 0;
	
	QString strCanId = "";
	QString strData = "";

	QMutex mutex;

	while( !m_bCancelled )
	{
#ifdef __CANON_
		unsigned long numMsgs = 1;
		PASSTHRU_MSG *pRxMsg = NULL;
		pRxMsg = theApp->GetMessageDetail( &numMsgs, 0 );
		nMsgAmount = numMsgs;  // store amount of messages we got
		
		strData = "";

		int index = 0;
		// Read can data, store in buffer ( read say N amount and then emit? )
		while ( numMsgs && !m_bCancelled )
		{
			mutex.lock();
			szTemp = "";
			m_can[index].strData = "";
			iCanId = 0;
			for ( int iLoop = 0; iLoop < 4; iLoop++)
			{
				iCanId = (iCanId << 8 ) | pRxMsg->Data[iLoop];
			}

			QString strId;
			strId.sprintf( "%d", iCanId );
			strCanId = strId;


			QString strName = theApp->m_dcb.GetMessageName( strId );
			if( strName != "" )
				m_can[index].strId = strName;
			else
			{
				if( pRxMsg->RxStatus & 0x100 )
				{
					m_can[index].strId.sprintf( "%X X", iCanId);
					strCanId.sprintf( "%X X", iCanId );
				}
				else
				{
					m_can[index].strId.sprintf( "%03X", iCanId);
					strCanId.sprintf( "%03X", iCanId );
				}
			}

			for ( int iLoop = 4; (unsigned)iLoop < (pRxMsg->DataSize ); iLoop++)
			{
				szTemp.sprintf("%02X ", pRxMsg->Data[iLoop]);
				m_can[index].strData += szTemp;
				strData += szTemp;
			}
			
			m_can[index].strTime.sprintf("%04d", pRxMsg->Timestamp);

			numMsgs--;

			m_nCount++;

			mutex.unlock();

		}  // end while ( ulNoMsgs )

	
#else  // some dummy data for testing 		
		QString strId = theApp->m_dcb.GetMessageName( "1911" );
		if( strId != "" )
			m_can[0].strId = strId;
		else
			m_can[0].strId = "0xfea";

		m_can[0].strData = "222222222222";
		m_can[0].strTime = "----";

		m_nCount += nMsgAmount;

		strCanId = "1911";
		strData = "2222222222";
#endif		
		// send out signal
		if( nMsgAmount > 0 )
			emit scrolltable( m_nCount - 2, nMsgAmount );
		msleep(10);
	
	} // end while ( true )

}

To copy to clipboard, switch view to plain text mode 

Kind regards,
Steve

[marcel]

What is the relation between m_can from the worker thread and the one from the GUI thread?

[steg90]

Hi Marcel,

The m_can structure is defined now in a header file and declared globally through out the application, use to be declared in the derived QMainWindow class.

The structure is :

#define __CANDATA
 
struct CANDATA
{
	QString strId;
	QString strTime;
	QString strData;
};
 
#endif
 
extern CANDATA			m_can[2];

#define __CANDATA

struct CANDATA
{
	QString strId;
	QString strTime;
	QString strData;
};

#endif

extern CANDATA			m_can[2];

To copy to clipboard, switch view to plain text mode 

And is declared externally.

Regards,
Steve

[marcel]

I see that the structure array has only 2 elements.
Are you sure that nAmount doesn't come > 2 from the thread?

[steg90]

100% sure

The crash appears to be in QtCored4.dll with some heap problem?


Crash is in the following function :

void __cdecl _free_base (void * pBlock)

void __cdecl _free_base (void * pBlock)

To copy to clipboard, switch view to plain text mode 

Found in free.c

line crash is on :

retval = HeapFree(_crtheap, 0, pBlock);

retval = HeapFree(_crtheap, 0, pBlock);

To copy to clipboard, switch view to plain text mode 

I'm at a loss, but like I said in previous posts, if I just pass QString's back as opposed to structure, all is fine...

Regards,
Steve

[marcel]

Anyway, you're using that mutex wrong.

You should create static getters and setters for that structure, and guard it with a mutex in there.

You're guarding entire portions of code right now.
I'm not saying this is the problem, but you should revise your code.

The problem could be that you emit that signal from a while loop.
They could be posted very fast/often in the GUI event loop, and maybe this is the reason it crashes.
Couldn't you queue the data that comes from the thread and apply it sequentially?

[steg90]

Thanks Marcel,

They are posted fast.

Are you saying to do something like this for the mutex :

struct CANDATA
{
	QString strId;
	QString strTime;
	QString strData;
 
                QMutex m_Mutex;
 
public:
                static QString getId() { return strId; }
                static QString getTime() { return strTime; }
                static QString getData() { return strData; }
 
                static void setId( QString id ) { m_Mutex.lock(); strId = id; m_Mutex.unlock();}
                static void setTime( QString time ) { m_Mutex.lock(); strTime = time; m_Mutex.unlock();}
                static void setData( QString data ) { m_Mutex.lock(); strData = data;m_Mutex.unlock(); }
 
};

struct CANDATA
{
	QString strId;
	QString strTime;
	QString strData;

                QMutex m_Mutex;

public:
                static QString getId() { return strId; }
                static QString getTime() { return strTime; }
                static QString getData() { return strData; }

                static void setId( QString id ) { m_Mutex.lock(); strId = id; m_Mutex.unlock();}
                static void setTime( QString time ) { m_Mutex.lock(); strTime = time; m_Mutex.unlock();}
                static void setData( QString data ) { m_Mutex.lock(); strData = data;m_Mutex.unlock(); }

};

To copy to clipboard, switch view to plain text mode 

Regards,
Steve

[marcel]

Yes, something like that, but keep in mind that a mutex can slow down things considerably.
You should make only one setter, with three parameters, and set all three members at once.

Regards

[steg90]

Thanks for that Marcel, I appreciate your help.

Regards,
Steve

[jpn]

Hmm, that will still allow reading and writing same time which is not safe (only concurrent writes are protected). I'd suggest using QReadLocker and QWriteLocker. Usage is extremely simple and it will allow both threads reading the values same time.

[wysota]

The mutex used in the first suggestion doesn't make sense. You create a local mutex and destroy it in the same method which means each call to the method creates a separate mutex, so there is no synchronisation.

@Marcel: Mutexes in Qt are extremely fast, so there won't be any noticable slowdown, provided that any thread doesn't hold the mutex longer than necessary.

[marcel]

Mutexes in Qt are extremely fast, so there won't be any noticable slowdown, provided that any thread doesn't hold the mutex longer than necessary

I know.
He had three setters and in each one of them he was locking/unlocking the mutex.
He was calling the methods one after another anyway, so I suggested combining them in a single method with only one lock.unlock.

[wysota]

Yes, you are right, of course. Using a read-write lock would speed up things even more without compromising safety.

class CANDATA 
// note "class" instead of "struct" here as structs are by default public
{
	QString strId;
	QString strTime;
	QString strData;
                QReadWriteLock m_Mutex;
public:
                QString getId() { QReadLocker(&m_Mutex); return strId; }
                QString getTime() { QReadLocker(&m_Mutex); return strTime; }
                QString getData() { QReadLocker(&m_Mutex); return strData; }
 
                void setId( QString id ) { QWriteLocker(&m_Mutex); strId = id; }
                void setTime( QString time ) { QWriteLocker(&m_Mutex); strTime = time; }
                void setData( QString data ) { QWriteLocker(&m_Mutex); strData = data;}
                void setAll(const QString &id, const QString &time, const QString &data){
                  QWriteLocker(&m_Mutex);
                  strId = id; strTime = time; strData = data;
                }
};

class CANDATA 
// note "class" instead of "struct" here as structs are by default public
{
	QString strId;
	QString strTime;
	QString strData;
                QReadWriteLock m_Mutex;
public:
                QString getId() { QReadLocker(&m_Mutex); return strId; }
                QString getTime() { QReadLocker(&m_Mutex); return strTime; }
                QString getData() { QReadLocker(&m_Mutex); return strData; }

                void setId( QString id ) { QWriteLocker(&m_Mutex); strId = id; }
                void setTime( QString time ) { QWriteLocker(&m_Mutex); strTime = time; }
                void setData( QString data ) { QWriteLocker(&m_Mutex); strData = data;}
                void setAll(const QString &id, const QString &time, const QString &data){
                  QWriteLocker(&m_Mutex);
                  strId = id; strTime = time; strData = data;
                }
};

To copy to clipboard, switch view to plain text mode 

BTW. Statics didn't make sense here, so I removed them.

[steg90]

Thanks guys,

I also removed the statics, and used the QReadLocker and QWriteLocker and all is now well.

Thank you all for your input and solutions

BTW, this is what my structured finished as :

struct CANDATA
{    
	QString strId;    
	QString strTime;    
	QString strData;                 
	QReadWriteLock m_lock; 
public:               
	QString getId() { QReadLocker locker(&m_lock); return strId; }
	QString getTime() { QReadLocker locker(&m_lock); return strTime; }
	QString getData() { QReadLocker locker(&m_lock); return strData; }
	void setData( QString id, QString time, QString data ) 
	{ 
		m_lock.lockForWrite(); 
		strId = id; 
		strTime = time; 
		strData = data; 
		m_lock.unlock();
	}               
};

struct CANDATA
{    
	QString strId;    
	QString strTime;    
	QString strData;                 
	QReadWriteLock m_lock; 
public:               
	QString getId() { QReadLocker locker(&m_lock); return strId; }
	QString getTime() { QReadLocker locker(&m_lock); return strTime; }
	QString getData() { QReadLocker locker(&m_lock); return strData; }
	void setData( QString id, QString time, QString data ) 
	{ 
		m_lock.lockForWrite(); 
		strId = id; 
		strTime = time; 
		strData = data; 
		m_lock.unlock();
	}               
};

To copy to clipboard, switch view to plain text mode 

Kind regards,
Steve

[jpn]

Default visibility of a struct is public (as wysota's comment mentions) so one can still accidentally access the members directly without using getters/setters.