Thread Safety for Pointers passed to CreateThread()

Question

0.00/5 (No votes)

See more:

Hi.

I am writing a Small Server app, which uses TCP/IP. In the classical style, i wait for 'WSAAccept' to unblock, and start a New thread, to process the Client Socket received.
I also use a Structure, to pass information on to the thread function.
Now, the Parent Thread may Loop again (and Modify the Structure to which 'CreateThread' got a pointer before the Thread Function has had a chance to take the values that the Param pointed at.

Below is a Code Snippet.

C++

struct THR_INFO{
	CSgSrvrCore* pThis;
	SOCKET hSocket;
};


typedef THR_INFO* LP_THR_INFO;

void Class::Function(){

	// Other Code

	THR_INFO* pThrInfo;

	DWORD dwThreadID;
	THR_INFO* pThrInfo;

	#ifdef PARANOID
		pThrInfo=new THR_INFO;
		pThrInfo->pThis=this;
		pThrInfo->hSocket=h_sockClient;
	#else

		THR_INFO ThrInfo={this,h_sockClient};
		pThrInfo=&ThrInfo;
	#endif

	CreateThread(0, 0, ConnectToClientThread, pThrInfo, 0, &dwThreadID);


	//Other Code
}

//static 
void Class::ConnectToClientThread(void* pParam){

	LP_THR_INFO pThrInfo=(LP_THR_INFO)p_Param;
	SOCKET h_sockClient=pThrInfo->hSocket;
	Class* pThis=pThrInfo->pThis;
	ASSERT(pThis);

	#ifdef PARANOID
         // The Item was Created with 'new'
		delete p_Param;
	#endif


}

Should I not be PARANOIDE, and the basic function will be thead safe, or should I include further Synchronisation Objects.

Regards :)

Posted 15-Apr-12 13:33pm

Bram van Kampen

Add a Solution

Comments

Nelek 15-Apr-12 19:42pm

Paranoia is good without abusing. As far as it doesn't affect performance or other aspects, I would say: if not sure... the more sugar, the sweeter.

[no name] 15-Apr-12 23:02pm

"Now, the Parent Thread may Loop again (and Modify the Structure to which 'CreateThread' got a pointer before the Thread Function has had a chance to take the values that the Param pointed at."

I don't understand this. With Paranoid on don't you instantiate a new structure object on the heap prior to each call to CreateThread. Thus there should be a 1-1 association between threads and structures. You show the thread then deleting the structure object. This would seem to be the whole point of the exercise.

Aescleal 16-Apr-12 5:44am

Be careful advising more synchronisation - you can cause all sorts of deadlocks if you don't know what you're doing. Actually most people introduce deadlocks when they know what their doing but that's another story.

1 solution

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

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

Chuck O'Toole · Accepted Answer · 2012-04-15T13:50:00

Solution 1

[edited to correct the fact that I misread something in the original code]

From the looks of it, it's "thread safe" if you turn on PARANOID

You are, in reality, passing a pointer by value to the created thread. That essentially creates a copy of the pointer on the local stack of the created thread so, in theory, any modification to the pointer itself in the main code (you do another "new") should not affect the copy owned by any thread (pParam). Now, that said, the "structure" that the pointer points to is, of course, something that you shouldn't modify once you pass the pointer to the created thread. Initialize the structure and then let it go. And you should "delete" the object when you are done with it. The main thread did the "new" and passed ownership / responsbility for the structure to the created thread. That thread should clean up after itself to avoid memory leaks.

With PARANOID turned off, you are instantiating your ThrInfo structure on the local stack of the "Function()" and passing the pointer to that. Local Stack structures can be temporary and go out of scope when the function exits so your thread has a pointer to some stack memory that may be overwritten by the main thread's further execution.

Cardinal Rule of Threading: Do not pass pointers to local stack objects, you will regret it.

Posted 15-Apr-12 13:50pm

Chuck O'Toole

Updated 15-Apr-12 13:56pm

v2

Comments

Bram van Kampen 15-Apr-12 20:44pm

Thanks,
With PARANOID turned on I thought I created a Pointer on the Local Stack, pointing to a Global Object This pointer is passed to the thread's stack, but still points to the same global object. I take it that all that is done when 'CreateThread' returns. MS Documentation states specfically that 'CreateThread' may return before the Newly Started thread completes( Otherwise 'CreateThread' would be pointless)or even before it commences!

I had Bad feelings about the code without 'PARANOIDE', and you confirm my bad feelings about it. The problem with synchronisation objects is, that I would have to apply the Lock in the Parent thread, and release it from the Child Thread. This is Something that does not sound right. (Having said that, maybe it is perfectly acceptable).

Apart from that, what I'm trying to do is not exactly breaking the mould. Useage of the 'CreateThread' function is peppered through all MS Apps.
I can only assume that they made it 'Usefull'. Unless if anyone comes up with a different Idea, I will for now assume that You're right in considering the 'PARANOIDE' option as threadsafe.

Thanks;

Regards :)

Chuck O'Toole 15-Apr-12 21:00pm

Well, to clarify your thoughts about CreateProcess(). You need to consider what you are passing as the argument. Consider the difference between passing "pThrInfo" and "&pThrInfo". In the first instance, you are passing the "contents" of "pThrInfo", in the second you would be passing the address of "pThrInfo". So, when CreateProcess() creates the argument list to pass to the created thread, it's going to put the argument on the local stack of the created process for the initial "call" to the thread. Passing the "contents" of the pointer, as you do in your code, removes any concern you have about the original pointer in the caller's code. If you passed the "address of the pointer" you would still rely on the pointer in the caller's code being "safe" until you did something with it in the created thread. Depending on the contents of the caller's variable requires synchronization objects.

JackDingler 16-Apr-12 18:06pm

As they are pointing out.

If you want to pass different data to each thread, then create a new instance of THR_INFO each time. Set up it's data. Then pass the pointer to the thread and let the thread delete it. In this model each thread will have it's own safe copy of the structure to read, with no fear it will randomly change.

If you pass the same pointer to every thread, and change the data it points to each time, then the data the thread will use, will be unpredictable.