Click here to Skip to main content
15,891,597 members
Please Sign up or sign in to vote.
5.00/5 (1 vote)
See more:
I have a some arbitrary data class that is in charge of locking out concurrent access:

CMyData.h:

C++
class CMyData
{
public:
	CMyData(void);
	virtual ~CMyData(void);
public:
	void Get(CMyDataItem** ppMyData, int& nSize);
	void Set(const CMyDataItem* pMyData, int nSize);
private:
	CMyDataItem* m_pMyData; // array of CMyDataItem
	int m_nTableSize;       // size of array
	CCriticalSection m_CriticalSection;
};


The implementations for Get and Set are:
C++
void CMyData::Get(CMyDataItem** ppMyData, int& nSize)
{
	CSingleLock lock(&m_CriticalSection, TRUE);
	if(NULL != m_pMyData)
	{
		if(m_nTableSize != nSize || NULL == *ppMyData)
		{
			// Table size has changed so rebuild
			delete[] *ppMyData;
			nSize = m_nTableSize;
			*ppMyData = new CMyDataItem[m_nTableSize];
		}
		CopyMemory(*ppMyData, m_pMyData, sizeof(CMyDataItem)*m_nTableSize);
	}
	else
	{
		ASSERT(0); // By context, shouldn't happen
	}
}


and

C++
void CMyData::Set(const CMyDataItem* pMyData, int nSize)
{
	CSingleLock lock(&m_CriticalSection, TRUE);
	if(NULL != pMyData)
	{
		if(m_nTableSize != nSize)
		{
				// Table size has changed so rebuild
				m_nTableSize = nSize;
				delete[] m_pMyData;
				m_pMyData = new CMyDataItem[nSize];
		}
		CopyMemory(m_pMyData, pMyData, sizeof(CMyDataItem)*nSize);
	}
	else
	{
		ASSERT(0); // by context, this shouldn't happen
	}
}


If I repeat Get and Set a few times, at some point, the content of m_pMyData turns into gobbledygook, and I promise I haven't got any other way of changing the content of m_pMyData!

Calling example:
C++
	CMyDataItem* pMyData = NULL;
	m_pMyData->Get(&pMyData);
	pMyData->m_nID = 10;
	m_pMyData->Set(pMyData);
	delete[] pMyData;
	pMyData = NULL;
// repeat...

Any ideas? Please help!

=============

PS:

Here is a bit more info.
class CMyDataItem
{
public:
	CMyDataItem();
	virtual ~CMyDataItem();
public:
	int m_nSomeID;
	int m_nAnotherID;
	int m_nNumber;
	double m_dValue1;
	double m_dValue2;
	double m_dValue3;
	double m_dValue4;
	double m_dValue5;
	double m_dValue6;
	double m_dValue7;
	double m_dValue8;
	double m_dValue9;
	bool m_bBool1;
	bool m_bBool2;
	bool m_bBool3;
	unsigned int m_uiMyID;
	CString m_strString1;
	CString m_strString2;
};
Posted
Updated 7-Aug-11 21:57pm
v3
Comments
Albert Holguin 8-Aug-11 1:15am    
Are you using CSingleLock correctly? See MSDN: http://msdn.microsoft.com/en-US/library/fw63hszf%28v=VS.100%29.aspx
PaulowniaK 8-Aug-11 1:28am    
Yup.

That bit seems OK.

It turns out, the copy memory bit is going wrong, but can't think why...
Albert Holguin 8-Aug-11 15:51pm    
When you want me to know you've replied to me, you need to write a comment using the "reply" in the top right of my comment... otherwise I receive no notification of this.
Peter_in_2780 8-Aug-11 1:52am    
From http://msdn.microsoft.com/en-us/library/bwk62eb7%28v=vs.80%29.aspx
"To use a CSingleLock object, call its constructor inside a member function in the controlled resource's class. Then call the IsLocked member function to determine if the resource is available. If it is, continue with the remainder of the member function. If the resource is unavailable, either wait for a specified amount of time for the resource to be released, or return failure. After use of the resource is complete, either call the Unlock function if the CSingleLock object is to be used again, or allow the CSingleLock object to be destroyed."

I don't see anything in your code other than the constructor call.

Peter
PaulowniaK 8-Aug-11 1:56am    
Thanks for your suggestion.
All I can say is that the usage I've shown is how we use CSingleLock in my workplace and it's been demonstrated to work fine in the past.

Also, commenting out the line does not help solve the problem I've observed.

(To test things a little by little, I've only used a single thread tester so commenting out the CSingleLock stuff doesn't cause any problems for the moment.)

1 solution

You should make sure CMyDataItem does not contain any pointers, or classes that in turn uses pointers, such as string classes or the like. The CopyMemory will ruin your data if it does. In that case you could use std::copy instead, and implement copy constructor of course.

I guess this is the most likely scenario since you are testing with a single thread.
 
Share this answer
 
v2
Comments
PaulowniaK 8-Aug-11 3:57am    
I updated the question to show what's inside CMyDataItem.

I can't see what could be causing the problem except for maybe the CStrings.
Niklas L 8-Aug-11 4:22am    
There you have it. You can't do a memcpy on those members. Add a copy constructor (and an assignment operator and possibly add move constructor if your compiler allows it). Skip CopyMemory and do a correct copy instance by instance, for example by using std::copy as I suggested.
PaulowniaK 8-Aug-11 4:24am    
Thanks!

Works now.
Albert Holguin 8-Aug-11 15:54pm    
Good catch Niklas, my 5
Niklas L 8-Aug-11 18:18pm    
Thanks Albert!

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900