Followers 0

# How to thread a class - Client Server Program

## 12 posts in this topic

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.

#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);

char buffer[VERY_LARGE_STRING];

private:

SOCKET listenSocket;
SOCKET clientSocket;

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

int sendLength;

ChatPacket* chatPacket;
};

Methods

#include "Server.h"

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

Server::~Server(void)
{
{
//set the Boolean to false so the thread will stop
// 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
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)
{
WSACleanup();
listenSocket = 0;
return 0;
}

// bind the TCP listening socket
if (r == INVALID_SOCKET)
{
closesocket(listenSocket);
WSACleanup();
listenSocket = 0;
return 0;
}

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

// start the accepting thread for connect requests

// start the receiving thread for receiving data

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;
}

{
Server* s = (Server*)t;

// set the Boolean to show the thread is running

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

short Server::Disconnect()
{

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

-4

##### Share on other sites
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.
1

##### Share on other sites

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
2

##### Share on other sites

Edited by TheComet
0

##### Share on other sites

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?

-2

##### Share on other sites

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.

-5

##### Share on other sites

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.

2

##### Share on other sites

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
1

##### Share on other sites

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?

2

##### Share on other sites

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.
0

##### Share on other sites

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 ),
sendLength( 0 )
/* -- etc -- */
{
}
Edited by TheComet
0

##### Share on other sites
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
0

## Create an account

Register a new account