Jump to content

  • Log In with Google      Sign In   
  • Create Account

How to thread a class - Client Server Program


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
12 replies to this topic

#1 codejockey   Members   -  Reputation: 77

Like
-4Likes
Like

Posted 24 November 2013 - 10:46 PM

So I've looked around with Bing search as to how to thread a class.  I see lot's of nice wrappers with lots of heavy duty pointer stashing and caching and said there's got to be an easier way.  Well here it is, what you all have been waiting for ... threading a class!!  It's so simple your just going to want to do it in every class you write.  I'll just put my server class here and comment where the thread dependent items are.  (btw, it's still a work-in-process but it does get a listen server and a client connection with data being passed)  The includes are just windows stuff and some simple buffers and not necessary to discuss.
 
Header

 

#pragma once

#include "includes.h"
#include "ChatPacket.h"

class Server
{
public:

	Server(void);
	~Server(void);

	// some normal methods
	short Server::Initialize(short port);
	short Server::Disconnect();
	short Server::Send();
	void Server::Listening();

	// these two methods are going to be the threads
	// notice the use of static and the (void* t) parameter
	static void Server::Accepting(void* t);
	static void Server::Receiver(void* t);

	char buffer[VERY_LARGE_STRING];

private:

	WSADATA wsaData;

	SOCKET listenSocket;
	SOCKET clientSocket;

	// some Booleans to keep track of which threads are active
	bool listenerIsActive;
	bool accepterIsActive;
	bool receiverIsActive;

	int receiveLength;
	int sendLength;

	// thread ID variables
	uintptr_t listenThread;
	uintptr_t acceptThread;
	uintptr_t receiveThread;
	uintptr_t sendThread;

	struct addrinfo *result, *ptr, hints;

	ChatPacket* chatPacket;
};

Methods

 

#include "Server.h"

Server::Server(void)
{
	memset(this, 0x00, sizeof(Server));
}

Server::~Server(void)
{
	if (receiverIsActive)
	{
		//set the Boolean to false so the thread will stop
		receiverIsActive = false;
		// wait for 1 second so the thread can stop
		Sleep(1000);
	}

	if(accepterIsActive)
	{
		// and again
		accepterIsActive = false;
		Sleep(1000);
		closesocket(clientSocket);
		clientSocket = 0;
	}

	if(listenerIsActive)
	{
		// and again for this one
		listenerIsActive = false;
		Sleep(1000);
		closesocket(listenSocket);
		listenSocket = 0;
	}

	WSACleanup();
}

short Server::Initialize(short port)
{
	// initialize winsock
	r = WSAStartup(MAKEWORD(2, 2), &wsaData);
	if (FAILED(r))
	{
		PrintDebugString("%s", DXGetErrorString(r));
		return 0;
	}

	memset(&hints, 0x00, sizeof(hints));
	hints.ai_family = AF_INET;
	hints.ai_socktype = SOCK_STREAM;
	hints.ai_protocol = IPPROTO_TCP;
	hints.ai_flags = AI_PASSIVE;

	// resolve this servers address and port
	// port has to be a character string ... who'da  thought that!
	char p[6];
	sprintf(p, "%i", port);

	r = getaddrinfo(NULL, p, &hints, &result);
	if (FAILED(r))
	{
		PrintDebugString("%s", DXGetErrorString(r));
		WSACleanup();
		return 0;
	}

	// create a SOCKET for this server
	listenSocket = socket(result->ai_family, result->ai_socktype, result->ai_protocol);

	if (listenSocket == INVALID_SOCKET)
	{
		freeaddrinfo(result);
		WSACleanup();
		listenSocket = 0;
		return 0;
	}

	// bind the TCP listening socket
	r = bind(listenSocket, result->ai_addr, (int)result->ai_addrlen);
	if (r == INVALID_SOCKET)
	{
		freeaddrinfo(result);
		closesocket(listenSocket);
		WSACleanup();
		listenSocket = 0;
		return 0;
	}

	freeaddrinfo(result);

	// start the listen server for connect requests
	Server::Listening();

	// start the accepting thread for connect requests
	acceptThread = _beginthread(&Server::Accepting, 0, this);

	// start the receiving thread for receiving data
	receiveThread = _beginthread(&Server::Receiver, 0, this);

	return 1;
}

void Server::Listening()
{
	// thought this might need to be threaded but turns out it doesn't
	// so toss the boolean out
	listenerIsActive = true;
	r = listen(listenSocket, SOMAXCONN);
	if (r == INVALID_SOCKET)
	{
		closesocket(listenSocket);
		WSACleanup();
		listenSocket = 0;
	}
}

void Server::Accepting(void* t)
{
	// using this as a thread must cast the this pointer
	// to something usable
	Server* s = (Server*)t;

	// set the Boolean to true so we know the thread is running
	s->accepterIsActive = true;

	// infinite loop until the Boolean is changed to false somewhere else in the class
	// i.e the deconstructor or the disconnect method
	while(s->accepterIsActive)
	{
		s->clientSocket = accept(s->listenSocket, 0, 0);
		if (s->clientSocket == INVALID_SOCKET)
		{
			closesocket(s->clientSocket);
			WSACleanup();
			s->clientSocket = 0;
			return;
		}
		// needed a spot to put a stopper for my debugger
		if (s->clientSocket > 0)
		{
			int zz = 0;
			zz++;  //  I put the stopper on this line
		}
	}
}

short Server::Send()
{
	// note-to-self don't use strlen(buffer) as there could be 0x00 somewhere in the buffer
	// and will not send the entire buffer if there is
	sendLength = send(clientSocket, buffer, strlen(buffer), 0);
	if (sendLength == SOCKET_ERROR)
	{
		PrintDebugString("Server::Send failed with error: %d", WSAGetLastError());
		closesocket(clientSocket);
		WSACleanup();
		return 0;
	}
	
	return 1;
}

void Server::Receiver(void* t)
{
	Server* s = (Server*)t;

	// set the Boolean to show the thread is running 
	s->receiverIsActive = true;

	// loop until this Boolean is false
	while(s->receiverIsActive)
	{
		if (s->clientSocket > 0)
		{
			memset(s->buffer, 0x00, VERY_LARGE_STRING);
			s->receiveLength = recv(s->clientSocket, s->buffer, VERY_LARGE_STRING, 0);
			if (s->receiveLength > 0)
			{
				// doing my client receive logic here
				PrintDebugString("Server::Bytes received: %d", s->receiveLength);
				if ((s->buffer[0] == 'C') && (s->buffer[1] == 'H'))
				{				
					s->chatPacket = new ChatPacket();
					memcpy(s->chatPacket, s->buffer, s->receiveLength);
					PrintDebugString("Server::CH: %s", s->chatPacket->message);
					delete s->chatPacket;
					s->chatPacket = 0;
				}
				memset(s->buffer, 0x00, VERY_LARGE_STRING);
			}
			else if (s->receiveLength == 0)
			{
				PrintDebugString("Server::No Data...");
			}
			else
			{
				PrintDebugString("Server::Recv failed: %d", WSAGetLastError());
				closesocket(s->clientSocket);
				WSACleanup();
				s->receiverIsActive = false;
				return;
			}
		}
	}
}

short Server::Disconnect()
{
	receiverIsActive = false;

	if(accepterIsActive)
	{
		accepterIsActive = false;
		Sleep(1000);
		closesocket(clientSocket);
		clientSocket = 0;
	}

	if(listenerIsActive)
	{
		listenerIsActive = false;
		Sleep(1000);
		closesocket(listenSocket);
		listenSocket = 0;
	}
	
	WSACleanup();

	return 1;
}

And that's all there is to threading with a class.  Don't bother with starting the thread outside of the class.  Too much chaos in doing that.  Simply call a method within the class to start the threading or do it in the class on its own with the constructor.  One final thought is that this server is set up to connect only one client so don't just cut and paste and assume its going to handle multiple clients (hint: s->clientSocket[x] or maybe put them in a link list?)

 

Microsoft's MSDN Links for Client Server Programming with Winsock

 

Getting Started With Winsock

 

Complete Server Code

 

Complete Client Code



Sponsor:

#2 BitMaster   Crossbones+   -  Reputation: 4140

Like
1Likes
Like

Posted 25 November 2013 - 05:11 AM

Some things that annoyed me without even starting to dig deeper:
1) using void as sole function parameter. You are using classes, that means C++ from the start. C calling conventions go out the window as step 1.
2) memset(this, ...). I don't care if you made sure this does not trigger undefined behavior. It's a horrible mess and the moment you need any polymorphism or any non-POD data member you suddenly have things blow up.
3) you do not prevent accidental copying of the Server class (private copy constructor/assignment operator, boost::noncopyable, = deleted declaration in C++11).

If I wanted threading I'd either go with boost::thread or std::thread (depending on preference and available C++11 capability). boost::asio is worth a look too. Unless you absolutely have to roll your own threading wrapper, I would strongly suggest not to (unless it is purely for educational purposes and never intended to be used in anything related to production code), especially at the level you appear to be at.

#3 haegarr   Crossbones+   -  Reputation: 4337

Like
2Likes
Like

Posted 25 November 2013 - 06:47 AM

Further flaws on the multi-threading and on the networking levels: A thread solely for accepting a connection? The main thread becomes blocked by listening for connections? An active wait synchronization between Receiver and Accepting? The client socket stored as member of the server object (also for demonstration purposes a bit strange)? No joining with terminated child threads?

 

It's so simple your just going to want to do it in every class you write.

Nonsense! Doing things just because they are possible is a No-Go. Resources would be wasted, and glitches accepted without any need. Keeping things as complex as needed but as simple as possible is the way to go.


Edited by haegarr, 25 November 2013 - 06:51 AM.


#4 TheComet   Members   -  Reputation: 1614

Like
0Likes
Like

Posted 25 November 2013 - 08:07 AM

memset( &thisThread, VERY_LARGE_RETARDATION, sizeof(thisThread) );


Edited by TheComet, 25 November 2013 - 08:27 AM.

YOUR_OPINION >/dev/null


#5 codejockey   Members   -  Reputation: 77

Like
-2Likes
Like

Posted 25 November 2013 - 10:14 PM

Ok with that being said.  Rewrite it in your way (going to have to assume that's the right way) so everyone can be clear on how it's supposed to be done.

 

If you know assembler then you'll know this is fine:

memset(this, 0x00, sizeof(Server));

 

Perhaps constructive criticism instead of this type of verbosity:

VERY_LARGE_RETARDATION

 

Did you guys even read the source in the class?  blink.png 



#6 Hodgman   Moderators   -  Reputation: 30432

Like
6Likes
Like

Posted 25 November 2013 - 11:10 PM

Everyone above is being harsh, and that's probably hurtful because you're proud to have figured out how to write a threaded server... but they're being harsh because the code is extremely flawed, and nobody should be copying it.


Server should be non-copyable as below. At the moment, if someone writes Server a, b; ... b = a; then your code will do bad things. The non-copyable idiom will create a compile-time error if a user is foolish enough to try and copy a Server object.

class Server
{
public:
//implemented in the CPP:
	Server();
	~Server();
private:
//not implemented - just decalred to be private:
	Server( const Server& );
	Server& operator=( const Server& );
};

There's no error-checking to see if Initialize is called twice.
An alternative to adding this error checking would be to get rid of the Initialize function and instead use the constructor (although this then almost requires you to use exceptions):

class Server
{
public:
	Server(int port);//throws ServerInitException
};

You don't need to declare your functions like this:

class Server
{
...
	short Server::Disconnect();
//This will do just fine:
	short Disconnect();

There's no point in using shorts for your function arguments and return values; It just makes things more complex. Use ints instead.

The static functions Accepting/Receiver should be private -- they are not meant to be called by users of the Server class. If a user does call them, it will likely break things.

buffer probably shouldn't be a member. Send should probably take buffer pointer/size arguments.

Send and Receiver probably shouldn't be sharing the same buffer...

memset(this...) is extremely bad practice in C++. If your class is non-POD, or any member variables are non-POD, then it's undefined behavior. If your class or any members have vtables, you'll be erasing them. It does happen to work for your class, but is still a very scary bit of code that you'd likely be shamed for in a professional environment! If you used that in a portfolio you would not be hired, and if you committed it on some jobs you could be fired, because it demonstrates a lack of understanding.

Sleeping for 1 second to wait for threads to finish is not guaranteed to work: it is possible for a thread to take longer than that to finish. Furthermore it's likely that it will take much less time than that, so you're sleeping for no reason. Instead, you should be joining with the thread / waiting on the thread's termination, which will sleep for just the right amount of time.

Your thread-shared variables (e.g. accepterIsActive) are completely not thread-safe. There's no reason that your loops such as "while(s->accepterIsActive)" couldn't be compiled into an infinite loop of "while(true)".

i.e. as is, it's entirely valid fro the compiler to replace your loop with while(true)!!!

It's only working for you due to luck. You need to use a synchronization primitive around the reads/writes of these variables.

 

Lastly - this whole exercise is unnecessary. You can just use non-blocking sockets on the main thread.



#7 codejockey   Members   -  Reputation: 77

Like
-5Likes
Like

Posted 25 November 2013 - 11:47 PM

This is getting annoying.  I only posted this because it shows how you can define the methods in a class to be threaded.  If you want it to be professionally used then I suggest doing all the changes that all these brilliant people pointed out to make it universally acceptable in the C++ programming community.  For me, it works, it does what I need, and isn't that actually what is important.  btw I wouldn't work anywhere that has C++ as its main programming language.  Get current with some managed code such as C# XNA or java.



#8 ChaosEngine   Crossbones+   -  Reputation: 2399

Like
2Likes
Like

Posted 26 November 2013 - 03:38 AM

codejockey, hodgman just used his own free time to write a detailed post explaining how you could improve your code. 

 

He's an excellent developer and you would do well to heed his advice instead of whining like a sulky teenager.


if you think programming is like sex, you probably haven't done much of either.-------------- - capn_midnight

#9 NightCreature83   Crossbones+   -  Reputation: 2853

Like
1Likes
Like

Posted 26 November 2013 - 08:54 AM

This is getting annoying.  I only posted this because it shows how you can define the methods in a class to be threaded.  If you want it to be professionally used then I suggest doing all the changes that all these brilliant people pointed out to make it universally acceptable in the C++ programming community.  For me, it works, it does what I need, and isn't that actually what is important.  btw I wouldn't work anywhere that has C++ as its main programming language.  Get current with some managed code such as C# XNA or java.

 

If you don't use thread synchronisation and memory barriers or fences you have a very real potential to generate race conditions, live or dead locks in your code. Only on very specific conditions can you omit a these synchronisation primitives, eg when you definitely know that the thread will only write to a certain part of the memory that no one else can touch at that point. This is generally not the case and to be thread safe you need fences/barriers or synchronisation primitives to safe guard this, as there is nothing stopping the compiler for seriously messing with the instruction order.

 

A fence, barrier or synchronisation primitive forces the compiler to not move any instruction before an acquire operation below that aqcuire, and for a release it guarantees that nothing will reorder up from below or out from in between the acquire and release. There is a whole lot of optimisation going on even after the compiler is done reordering stuff, out of order CPU's can also mess with this instruction order and here these promitives will guarantee the CPU will do the same thing as the compiler around them.

 

Concurence is one of the hardest problems to solve in CS and as soon as you go beyond the trivial example it becomes extremely complex.
 

Even in managed code you need these operations to guarantee all of what has been mentioned in this thread about thread safety and correct operation of a multi-threaded application.

 

Threading an application comes with a very big warning that you need to be extremely careful with what you are doing especially around shared memory locations.


Edited by NightCreature83, 26 November 2013 - 08:57 AM.

Worked on titles: CMR:DiRT2, DiRT 3, DiRT: Showdown, GRID 2, Mad Max

#10 Pink Horror   Members   -  Reputation: 1204

Like
2Likes
Like

Posted 26 November 2013 - 08:45 PM


I wouldn't work anywhere that has C++ as its main programming language.  Get current with some managed code such as C# XNA or java.

 

Are you just trolling now? If you wouldn't use C++, why are you sharing C++ code with the rest of us?



#11 King Mir   Members   -  Reputation: 2002

Like
0Likes
Like

Posted 27 November 2013 - 07:34 PM

This is getting annoying.  I only posted this because it shows how you can define the methods in a class to be threaded.  If you want it to be professionally used then I suggest doing all the changes that all these brilliant people pointed out to make it universally acceptable in the C++ programming community.  For me, it works, it does what I need, and isn't that actually what is important.  btw I wouldn't work anywhere that has C++ as its main programming language.  Get current with some managed code such as C# XNA or java.

But the code doesn't define methods to be threaded. It works on your particular machine, on your particular compiler settings, for the tests you did on it, but it's not correct code. Sharing memory without synchronization primitives is undefined behavior in C++, also called "catch fire semantics" because the code is legally allowed by the C++ standard to do anything including make your computer catch fire. Furthermore, this code is the worst kind of erroneous behavior, because it works most of the time, and doesn't give an easy to trace error when it doesn't, making the errors very hard to track down.

#12 TheComet   Members   -  Reputation: 1614

Like
0Likes
Like

Posted 28 November 2013 - 05:12 AM

If you know assembler then you'll know this is fine:

memset(this, 0x00, sizeof(Server));

 

It has nothing to do with assembler. And no, that's the most atrocious, evil piece of code I've ever seen and belongs in the "Coding Horrors" section of this forum.

 

The correct thing to do is use an initialiser list in the constructor:

Server::Server() :
    listenerIsActive( false ),
    accepterIsActive( false ),
    receiverIsActive( false ),
    receiveLength( 0 ),
    sendLength( 0 )
    /* -- etc -- */
{
}

Edited by TheComet, 28 November 2013 - 05:13 AM.

YOUR_OPINION >/dev/null


#13 BitMaster   Crossbones+   -  Reputation: 4140

Like
0Likes
Like

Posted 28 November 2013 - 06:33 AM

I wouldn't call it just that. For example, by adding the simple line
static_assert(std::is_pod<Server>::value);
would probably make it safe. I still wouldn't do it myself, but the compiler should scream at you instead of silently setting fire to the system.

My problem was less with the memset as such as more with the complete thoughtlessness and ignorance in putting it there. If you absolutely have to put something like that in there, at least comment the inherent dangers properly. If possible (not without C++11 support) also add compile time check.

Edited by BitMaster, 28 November 2013 - 07:13 AM.





Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS