[woppix]

I'm having a few issues figuring out how to use vectors to handle multiple Winsock clients. Below is what I'm currently trying to do, This works with one client just not multiple ones. I'm pretty much just making a chatroom type thing. When one client connects everything works fine, when another connects the first stops working and the second one works. Also the thread is created each time a client connects to handle that specific client. I'll also post the socket class below. If it will help you to have the entire code, just let me know and I'll upload it.

case FD_ACCEPT:
	{
		//create socket for the client
		CSockets ClientSocket;

		//add the client socket to the Cons vector to work
		//with multiple client. 
		Cons.push_back(ClientSocket);

		//accept the client connection
		if (ServerSocket.Accept(Cons.at(i)))
		{
			SetWindowText(hEditRecv, "**Client Connected**");
		}

		Cons.at(i).connected = true;

		//create a new thread and pass in the ClientSocket
		hClientThreadHandle = CreateThread(
			NULL,
			NULL,
			(LPTHREAD_START_ROUTINE) &thrClientThread,
			&Cons.at(i),
			NULL,
			&dwClientID
			);

		i++;

			
	}break;

CSockets.h

#ifndef _CSockets_H_
#define _CSockets_H_

///////////////////////////////////////////////
//Class to handle all aspects of sockets
////////////////////////////////////////////////

#include <winsock2.h>
#include <stdio.h>

class CSockets
{
private:
	int nResult;

public:
	SOCKET skSocket;
	int iStatus;
	bool connected;
	//WSADATA wsaData;

	CSockets();
	~CSockets();

	int Bind(int iPort);
	bool Initialize();
	int Listen( void );
	bool Connect (char* szServerAddress, int iPort);
	bool Accept (CSockets& skAcceptSocket);

	int Recv(char *szBuffer, int iBufLen, int iFlags);
	int Send(char *szBuffer, int iBufLen, int iFlags);

};


#endif

CSockets.cpp

#include "CSockets.h"

//constructor
CSockets::CSockets()
{
	
	nResult = 0;
	WSADATA wsaData;
	WORD wVersionRequested;

	wVersionRequested = MAKEWORD(2, 0);

	skSocket = INVALID_SOCKET;
	iStatus = WSAStartup(wVersionRequested, &wsaData);


}

//deconstructor
CSockets::~CSockets()
{


}

int CSockets::Listen( void )
{
	return listen(skSocket, SOMAXCONN);
}

int CSockets::Bind(int iPort)
{
	sockaddr_in saServerAddress;

	skSocket = socket(AF_INET, SOCK_STREAM, 0);
	
	if(skSocket == INVALID_SOCKET)
	{
		return false;
	}

	memset(&saServerAddress, 0, sizeof(sockaddr_in));

	saServerAddress.sin_family = AF_INET;
	saServerAddress.sin_addr.s_addr = htonl(INADDR_ANY);
	saServerAddress.sin_port = htons(iPort);

	if( bind(skSocket, (sockaddr*) &saServerAddress, sizeof(sockaddr)) == SOCKET_ERROR)
	{
		//Disconnect();
		return false;
	}
	else
		return true;
}

bool CSockets::Accept(CSockets &skAcceptSocket)
{
	sockaddr_in saClientAddress;
	int			iClientSize = sizeof(sockaddr_in);
	SOCKADDR	IPAddress;

	skAcceptSocket.skSocket = accept( skSocket, (struct sockaddr*)&saClientAddress, &iClientSize );
	
	if( skAcceptSocket.skSocket == INVALID_SOCKET ) 
	{
		return false;
	}
	else 
	{
		return true;
	}
}

int CSockets::Send(char *szBuffer, int iBufLen, int iFlags)
{
	return send(skSocket, szBuffer, iBufLen, iFlags);
}

int CSockets::Recv(char *szBuffer, int iBufLen, int iFlags)
{
	return recv(skSocket, szBuffer, iBufLen, iFlags);
}

bool CSockets::Connect(char* szServerAddress, int iPort)
{
	struct		sockaddr_in serv_addr;
	LPHOSTENT	lphost;

	memset(&serv_addr,0,sizeof(sockaddr_in));
	serv_addr.sin_family = AF_INET;
	serv_addr.sin_addr.s_addr = inet_addr(szServerAddress);

	if (serv_addr.sin_addr.s_addr == INADDR_NONE)
	{
		lphost = gethostbyname(szServerAddress);
		if (lphost != NULL)
			serv_addr.sin_addr.s_addr = ((LPIN_ADDR)lphost->h_addr)->s_addr;
		else
		{
			WSASetLastError(WSAEINVAL);
			return FALSE;
		}
	}

	serv_addr.sin_port = htons(iPort);

	// Open the socket
	skSocket = socket(AF_INET, SOCK_STREAM, 0);
	if(skSocket == INVALID_SOCKET)
	{
		return false;
	}

	int err = connect(skSocket, (struct sockaddr*)&serv_addr,sizeof(sockaddr));
	if(err == SOCKET_ERROR)
	{
		//Disconnect();
		return false;
	}

	return true;
}

[Salem_c]

> 22 (LPTHREAD_START_ROUTINE) &thrClientThread,
> 23 &Cons.at(i),
1. Where is your code for thrClientThread ?
2. Passing a pointer to the inside of a vector to a thread is dangerous!!!
You're basically relying on vector NEVER moving that allocated spot no matter how many items you push into the vector.

STL objects are not thread safe.

You need something like

// This contains info to be sent from the master to the child threads.
struct conInfo {
   SOCKET   s;
   // anything else you need to pass from one thread to another.
};

...

conInfo *inf = new conInfo;
inf->s = whatever;
inf->anything_else = whatever;

Then replace your "&Cons.at(i)" with the "inf" pointer.

The spawned thread takes over that memory and delete's it when it's done.

[woppix]

currently this is what i have for my thread function... its my first time messing with threads sooo. What I'm basically trying to accomplish with the thread is for it to be able to receive data that the client sends.

void thrClientThread(CSockets *Client)
{
	//buffers to hold data
	char szRecvBuffer[256];
	char szHistoryBuffer[10000];

	//variable to hold bytes received
	int iBytesRecv = 0;
	int iBytesSent = 0;

	//clear out the buffers
	ZeroMemory(szRecvBuffer, sizeof(szRecvBuffer));
	ZeroMemory(szHistoryBuffer, sizeof(szHistoryBuffer));

	while(TRUE)
	{
		//if the client is connected
		if (Client->connected)
		{
			//check to see if any data has been received
			iBytesRecv = Client->Recv(szRecvBuffer, sizeof(szRecvBuffer), 0);

			//if data was received
			if (iBytesRecv != -1)
			{
				strncat(szHistoryBuffer, szRecvBuffer, iBytesRecv);
				strcat(szHistoryBuffer, "\r\n");

				//output received data
				SendMessage(hEditRecv, WM_SETTEXT, sizeof(szRecvBuffer)-1, reinterpret_cast<LPARAM>(&szHistoryBuffer));

				//echo the data to all clients
				//(char array is sometimes not sending correctly on client)
				for (int i = Cons.size()-1; i >= 0; i--)
				{
					iBytesSent = Client->Send(szRecvBuffer, strlen(szRecvBuffer), 0);
				}
			}
		}
	}
}

This post has been edited by woppix: 17 February 2011 - 08:12 AM

[Salem_c]

What stops szHistoryBuffer overflowing?

> SendMessage(hEditRecv, WM_SETTEXT, sizeof(szRecvBuffer)-1, reinterpret_cast<LPARAM>(&szHistoryBuffer));
Why the mis-match between szRecvBuffer and szHistoryBuffer?
Are you setting the WHOLE text every time, or do you just want to add the most recently received text?

> iBytesSent = Client->Send(szRecvBuffer, strlen(szRecvBuffer), 0);
Since you don't manage to write a \0 in the correct place, doing strlen on it is a bug trap.

> iBytesRecv = Client->Recv(szRecvBuffer, sizeof(szRecvBuffer), 0);
If this also FILLS the buffer completely, there is no room to append a \0 to begin with.

[woppix]

thanks for pointing all of this out.

>it works like a chat so i really just need to sent the most recent data and then add that data to a edit box on the clients. I'm guessing i just need to completely get rid of szHistoryBuffer?

>so i basically just need to do a strcat like...
strcat(szRecvBuffer, "\0");
and then send the data?

>as for the other problem, not quite sure i know what to do about it.


And for the post about the structure and vectors being a bad choice. I just need to get rid of the vector totally and make my accept function in the socket class return the socket, and then assign it to inf->s?

This post has been edited by woppix: 17 February 2011 - 08:08 AM

[Salem_c]

To make sure your receive buffer always has a \0, do this

iBytesRecv = Client->Recv(szRecvBuffer, sizeof(szRecvBuffer)-1, 0);
if ( iBytesRecv > 0 ) {
  szRecvBuffer[iBytesRecv] = '\0';
} else
if ( iBytesRecv == 0 ) {
  // connection closed
} else {
  // error
}

I don't know why you should be appending \r\n to every buffer.

TCP is a stream protocol, meaning that only the order is preserved. Packet boundaries can get scrambled.
So you might send "Hello world\r\n", but you might have to call recv() twice to get say "Hel" in one call, and "lo world\r\n" in the next call.

Appending a newline for each message would scramble the text on screen with random unnecessary newlines.

[woppix]

thanks a lot for the help, i haven't quite got to implementing the structure yet. I did change up my thread and prevented data from overflowing and that part seems to be working now. Going to try to do as you said and put the structure in/remove the vector, and see if I can get that working. If i run into problems i may post back to be sure I'm on the right track, but want to try that myself for a few hours first and see if I can make it work.

This post has been edited by woppix: 17 February 2011 - 08:53 AM

[woppix]

instead of using a structure will it work the same way just to use my class object? Seems like it would perform basically the same thing and at the same time make accessing my Socket functions easier.

So instead of something like below, just dynamically create a object of my CSockets class and pass that to the thread function?

// This contains info to be sent from the master to the child threads.
struct conInfo {
SOCKET   s;
 // anything else you need to pass from one thread to another.
};

...
	 
conInfo *inf = new conInfo;
inf->s = whatever;
inf->anything_else = whatever;

This post has been edited by woppix: 17 February 2011 - 11:06 AM

[Salem_c]

The way to get a class instance into a thread goes something like this.

#include <iostream>
#include <iomanip>
#include <pthread.h>

using namespace std;

class receiver {
  public:
    receiver() : socket(0) { cout << "Default ctor =" << socket << endl; }
    receiver(int sock) : socket(sock) { cout << "Value ctor = " << socket << endl; }
    ~receiver() { cout << "Dtor" << endl; };
    static void* rxThreadWrapper(void*);
    void *rxThread(void);
  private:
    int socket;
};

// The instance is the parameter.
// We cast this into the appropriate type, and invoke a normal
// member function.
void* receiver::rxThreadWrapper ( void *instance ) {
  receiver  *r = reinterpret_cast<receiver*>(instance);
  void *res = r->rxThread();
  delete r;     // delete here, if you're not interested past this point
  return res;   
}

// A normal class member function with implicit 'this' pointer
void *receiver::rxThread ( void ) {
  cout << "Receiver thread called with sock = " << socket << endl;
  return NULL;
}

int main ( ) {
  receiver  *rx1 = new receiver(42);
  pthread_t tid;
  int n = pthread_create(&tid,NULL,receiver::rxThreadWrapper,rx1);
  cout << "n=" << n << endl;
  void *res;
  n = pthread_join(tid, &res);
  cout << "n=" << n << endl;
//  delete rx1;
  return 0;
}

Note the static member function which forms a bridge between the anonymous void* of threads with the class member functions with a typed 'this' pointer.

[woppix]

thank you for the explaining. I could be wrong but i don't think that would work in this case because i want the server to be able to handle an unlimited amount of connections, which is why i was trying to do it with vectors. I also need to be able to loop through the client sockets to send data from the server to all clients. What would you recommend me doing? I basically just want unlimited connections and to still be able to use my socket class to handle all the winsock stuff.

This post has been edited by woppix: 17 February 2011 - 07:15 PM

[woppix]

I was playing with the code some more today.. came up with an alternative way to do this, but with this method I don't know how to do one thing. When a client sends data to the server, the server should echo that data to all clients. Any idea how to add in that functionality.. Also would this method work to use? It seems like this would solve the memory problem from using vectors, and I can connect multiple clients. If i can't echo the data to all the clients and allow the server to send data to all clients then its pointless to continue doing it like this. In the previous code I had a vector of connections to loop through and then send the data to all of them, now since i don't have anything to loop through... I'm not sure what to do.

struct conInfo
{
   CSockets Socket;

};

case FD_ACCEPT:
	{

		conInfo *inf = new conInfo;

		//create socket for the client
		CSockets ClientSocket;

		//accept the client connection
		if (ServerSocket.Accept(ClientSocket))
		{
			vShowText(hListRecv, "**Client Connected**");
			//Cons.at(i).connected = true;
			//SetWindowText(hEditRecv, "**Client Connected**");
		}

		//set socket in structure to equal the accepted socket
		inf->Socket.skSocket = ClientSocket.skSocket;

		//create a new thread and pass in the ClientSocket
		hClientThreadHandle = CreateThread(
			NULL,
			NULL,
			(LPTHREAD_START_ROUTINE) &thrClientThread,
			inf,
			NULL,
			&dwClientID
			);

	}break;

void thrClientThread(conInfo *inf)
{
	//buffers to hold data
	char szRecvBuffer[256];
	//char szHistoryBuffer[10000];

	//variable to hold bytes received
	int iBytesRecv = 0;
	int iBytesSent = 0;

	//clear out the buffers
	ZeroMemory(szRecvBuffer, sizeof(szRecvBuffer));
	//ZeroMemory(szHistoryBuffer, sizeof(szHistoryBuffer));

	while(TRUE)
	{
		//if the client is connected
		//if (Client->connected)
		//{
			//check to see if any data has been received
			iBytesRecv = inf->Socket.Recv(szRecvBuffer, sizeof(szRecvBuffer)-1, 0);

			if (iBytesRecv > 0)
			{
				szRecvBuffer[iBytesRecv] = '\0';

				vShowText(hListRecv, szRecvBuffer);

				//for (int i = Cons.size()-1; i >= 0; i--)
				//{
				iBytesSent = inf->Socket.Send(szRecvBuffer, strlen(szRecvBuffer), 0);
				//	iBytesSent = Client->Send(szRecvBuffer, strlen(szRecvBuffer), 0);
				//}
			}

	}
}

This post has been edited by woppix: 19 February 2011 - 03:31 AM

[woppix]

I believe i have this issue solved, have to do a few more tests but it seems to be working.