[Rixterz]

Hi

I've been trying for hours to create a server which accepts multiple connections, assigns a class for each connection, and can use them later to send data to each one.

What I have so far:

ClientConnection.cpp

#include "stdafx.h"
#include "ClientConnection.h"

ClientConnection::ClientConnection()
{

}

ClientConnection::~ClientConnection()
{

}

ClientConnection.h

#pragma once

class ClientConnection
{
public:
	ClientConnection();
	~ClientConnection();

};

GameServer.cpp

#include "stdafx.h"
#include <windows.h>
#include <stdlib.h>
#include <stdio.h>
#include <winsock.h>
#include <vector>
#include "GameServer.h"

int port = 1123;

GameServer::GameServer()
{
	WSADATA wsaData;
	sockaddr_in server;
	SOCKET listenSocket;
	DWORD clientConnection;

	// start highest version of winsock
	if (WSAStartup(0x101, &wsaData) != 0) return;
	
	// fill winsock struct
	server.sin_family = AF_INET;
	server.sin_addr.s_addr = INADDR_ANY;
	server.sin_port = htons(port);

	// create listen socket
	listenSocket = socket(AF_INET, SOCK_STREAM, 0);

	if (listenSocket == INVALID_SOCKET) return;
	
	// bind socket to port
	if (bind(listenSocket, (sockaddr*)&server, sizeof(server)) != 0) return;
	
	// listen for a connection  
	if (listen(listenSocket, 5) != 0) return;
	
	SOCKET clientSocket;
	sockaddr_in from;
	int fromlen = sizeof(from);

	while (true)
	{
		// accept connections
		clientSocket = accept(listenSocket, (struct sockaddr*)&from, &fromlen);
		printf("Client connected\r\n");

		// create new thread and pass socket
		CreateThread(NULL, 0, ListenThread, (LPVOID)clientSocket, 0, &clientConnection);
	}

	// shutdown winsock
	closesocket(listenSocket);
	WSACleanup();
}

GameServer::~GameServer()
{

}

DWORD WINAPI GameServer::ListenThread(LPVOID socket)
{
    ...
}

GameServer.h

#pragma once

class GameServer
{
public:
	GameServer();
	~GameServer();

private:
	DWORD WINAPI ListenThread(LPVOID socket);

};

On this line,

CreateThread(NULL, 0, ListenThread, (LPVOID)clientSocket, 0, &clientConnection);

I'm getting the errors:

'GameServer::ListenThread': non-standard syntax; use '&' to create a pointer to member
argument of type "DWORD (__stdcall GameServer::*)(LPVOID socket)" is incompatible with parameter of type "LPTHREAD_START_ROUTINE"

[Skydiver]

This isn't an issue of multithreading and C++ fighting each other. This is an issue of you not understanding that your ListenThread() method is an instance method which takes an implicit this pointer as the first parameter ahead of the formal parameters that you listed. So from the compilers point of view, it looks like:

DWORD WINAPI ListenThread(GameServer * this, LPVOID socket);

Obviously, that doesn't fit the function signature for the thread start routine.

The usual approach is to declare a static method that takes a void * parameter so that you end up with a class method instead of an instance method. When calling the CreateThread() function, you pass in the class method for lpStartAddress and the instance as the lpParameter. Inside the class method, you cast the lpParameter back into the class, and then invoke an instance method on the class.

For an example of a similar approach for dealing with the WinProc see my old tutorial: Better Windows Code Organization Using C++ classes

[Rixterz]

Why does the instance need to be passed to the method if the method is declared within a class which will be instantiated later anyway?

This is so confusing to me.

Now in my .h I have:

DWORD WINAPI ListenThread(LPVOID socket);
static DWORD WINAPI a(void* b, LPVOID socket);

.cpp:

CreateThread(NULL, 0, a, this, 0, &clientConnection);

DWORD WINAPI GameServer::a(void* b, LPVOID socket)
{
	GameServer s = (GameServer)b;
	return s.ListenThread(socket);
}

DWORD WINAPI GameServer::ListenThread(LPVOID socket)
{
...
}

This post has been edited by Rixterz: 04 December 2017 - 04:34 PM

[Skydiver]

I don't understand why you considered my previous post unhelpful when I was telling you exactly what was going wrong and why as well as offered up a suggestion a long with some similar code on how to fix things.

[Skydiver]

Anyway, your GameServer:a() does not fit the signature of a thread procedure. Recall that it should only have one parameter.

Inside that same function you are casting a void pointer into a class instance. That's not going to work. You'll want to cast the pointer to class instance pointer.

[Skydiver]

Rixterz, on 04 December 2017 - 06:12 PM, said:

Why does the instance need to be passed to the method if the method is declared within a class which will be instantiated later anyway?

For the same reason in C# where static methods don't have access to instance variables and methods.

Sample C#:

class Foo
{
    int _id;

    void Update() { }

    static void ThisWorks(Foo foo)
    {
        foo._id = 23;
        foo.Update();
    }

    static void ThisDoesNotCompile()
    {
        _id = 29; // needs instance
        Update(); // needs instance
    }
}

[Rixterz]

Ok, so why is ListenThread static in the first place? It’s just a method which should be used like any other

Now I have:

.h:

#pragma once

class GameServer
{
public:
	GameServer();
	~GameServer();

private:
	static DWORD WINAPI StartListenThread(InstanceSocketStruct s);
	DWORD WINAPI ListenThread(LPVOID socket);
	struct InstanceSocketStruct;

};

.cpp:

GameServer::InstanceSocketStruct s;
s.instance = this;
s.socket = (LPVOID)clientSocket;

CreateThread(NULL, 0, StartListenThread, s, 0, &clientConnection);

...

DWORD WINAPI GameServer::StartListenThread(InstanceSocketStruct s)
{
	s->instance->ListenThread(s->socket);
}

DWORD WINAPI GameServer::ListenThread(LPVOID socket)
{
        ...
}

struct InstanceSocketStruct
{
	GameServer* instance;
	LPVOID socket;

	InstanceSocketStruct(GameServer* instance, LPVOID socket)
	{
		this->instance = instance;
		this->socket = socket;
	}
};

And 11 errors relating to:

• incomplete type not allowed
• undefined struct

This post has been edited by Rixterz: 05 December 2017 - 02:09 AM

[Skydiver]

Rixterz, on 05 December 2017 - 03:28 AM, said:

And 11 errors relating to:

• incomplete type not allowed
• undefined struct

In the future post the complete errors without summarizing or trimming content. There is important information in the warnings and error messages that makes it easier to diagnose problems.

In general, the problem is that you did not fully define the struct InstanceSocketStruct in your header file. In C and C++, most of the time, you need to fully define things before you use them. (The other times, you can forward declare things and then declare pointers to them, but that is a different topic for another time.)

From my point of view, you don't really need struct InstanceSocketStruct in the header file. You only need it within the .CPP file and declared at a global scope.

Although you are on the right track with regards to the incoming parameter for StartListenThread() is a pointer to a InstanceSocketStruct, you should comply with the signature for a ThreadProc, by declaring StartListenThread() as taking a void pointer. Yes, you can coerce the compiler into accepting it as the lpStartAddress parameter but in C++, the less casting the better because you want the compiler to catch errors when you pass things of the wrong type. Whenever you cast something using a C style cast, you are telling the compiler "Trust me. I know what I am doing."

[Skydiver]

Rixterz, on 05 December 2017 - 03:28 AM, said:

Ok, so why is ListenThread static in the first place? It’s just a method which should be used like any other

Unfortunately, it is not. There is a difference between member functions and static class functions. Member functions are what C# calls member methods, or Java what calls instance methods. Static class functions are what C# calls static methods, or what Java calls class methods.

Member functions "know" which instance they are acting on. Class functions don't know what instance they are acting on. The way that a member function "knows" which instance it is acting on is by the compiler always silently passing in the this pointer as the first parameter. The compiler does not do this for static class functions. The function call to a static class function looks like like a regular function call.

ListenThread needs to be static because the lpStartAddress parameter of CreateThread expects a function that is called using the regular C calling convention. Recall that parameter is declared as LPTHREAD_START_ROUTINE. If you dig through MSDN, or the header files, you see that:

typedef DWORD (__stdcall *LPTHREAD_START_ROUTINE) (  
    [in] LPVOID lpThreadParameter  
);  

The __stdcall is what hidden by the WINAPI macro that you normally see. The WINAPI calling convention is to call methods the Pascal way instead of the C or C++ way.

[Rixterz]

Ok, so now I have:

CreateThread(NULL, 0, StartListenThread, new InstanceSocketStruct(this, (LPVOID)clientSocket), 0, &clientConnection);

DWORD WINAPI GameServer::StartListenThread(void* s)
{
	InstanceSocketStruct* st = (InstanceSocketStruct*)s;
	st->instance->ListenThread(st->socket);
}

DWORD WINAPI GameServer::ListenThread(LPVOID socket)
{
	// retrieve passed socket
	SOCKET clientSocket = (SOCKET)socket;
}

struct InstanceSocketStruct
{
	GameServer* instance;
	LPVOID socket;

	InstanceSocketStruct(GameServer* instance, LPVOID socket)
	{
		this->instance = instance;
		this->socket = socket;
	}
};

I removed the struct from the header so now I have:

private:
	static DWORD WINAPI StartListenThread(void* s); // not sure how I know I'm supposed to use WINAPI here, or whether static is required for both
	static DWORD WINAPI ListenThread(LPVOID socket);

However this still produces errors;

• CreateThread does not take 3 arguments
• InstanceSocketStruct: undeclared identifier
• st: undeclared identifier
• Syntax error ')' on line 'InstanceSocketStruct* st = (InstanceSocketStruct*)s;'
• Syntax error: identifier 'InstanceSocketStruct'

This post has been edited by Rixterz: 05 December 2017 - 03:34 PM

[Skydiver]

Yes, StartListenThread() should be static and WINAPI. Your ListenThread() should not be static.

As I previously said, paste the exact errors you are getting. Don't summarize. Don't trim out information.

As I also previously said, you need to declare things in C++ before you use them. C++ is not as smart as C# where you can declare things later. Based on the snippet you posted above, it looks like you defined your struct InstanceSocketStruct after you used it in your call to CreateThread().

[Rixterz]

Alright, that cleared up most of the errors. The only thing it's complaining about now is:

'GameServer::StartListenThread': must return a value

I tried:

return st->instance->ListenThread(st->socket);

but that gives an ugly list of 9 unresolved externals, I'm sure that's not the way.

This post has been edited by Rixterz: 05 December 2017 - 04:03 PM

[Skydiver]

Assuming you declared ListenThread() as:

DWORD ListenThread(LPVOID socket);

that should have fixed the compilation error regarding "must return a value".

Until you get clean compilation done, don't stress the linking errors regarding unresolved externals because not all your code is compiling yet.

Once everything compiles properly, then look at the linking problems. Very likely you just forget to link in some libraries. Again with you not posting the exact errors and always summarizing or trimming the errors, it is very hard to help you. Everything becomes guesswork, and we may actually be giving you bad advice, or advice that doesn't match your actual problem.

[Rixterz]

I've declared it correctly but the problem persists. The full code is:

.h

#pragma once

class GameServer
{
public:
	GameServer();
	~GameServer();

private:
	static DWORD WINAPI StartListenThread(void* s);
	DWORD WINAPI ListenThread(LPVOID socket);

};

.cpp

#include "stdafx.h"
#include <windows.h>
#include <stdlib.h>
#include <stdio.h>
#include <winsock.h>
#include <vector>
#include "GameServer.h"

int port = 1123;

struct InstanceSocketStruct
{
	GameServer* instance;
	LPVOID socket;

	InstanceSocketStruct(GameServer* instance, LPVOID socket)
	{
		this->instance = instance;
		this->socket = socket;
	}
};

GameServer::GameServer()
{
	WSADATA wsaData;
	sockaddr_in server;
	SOCKET listenSocket;

	// start highest version of winsock
	if (WSAStartup(0x101, &wsaData) != 0) return;
	
	// fill winsock struct
	server.sin_family = AF_INET;
	server.sin_addr.s_addr = INADDR_ANY;
	server.sin_port = htons(port);

	// create listen socket
	listenSocket = socket(AF_INET, SOCK_STREAM, 0);

	if (listenSocket == INVALID_SOCKET) return;
	
	// bind socket to port
	if (bind(listenSocket, (sockaddr*)&server, sizeof(server)) != 0) return;
	
	// listen for a connection  
	if (listen(listenSocket, 5) != 0) return;
	
	SOCKET clientSocket;
	sockaddr_in from;
	int fromlen = sizeof(from);

	while (true)
	{
		// accept connections
		clientSocket = accept(listenSocket, (struct sockaddr*)&from, &fromlen);
		printf("Client connected\r\n");

		// create new thread and pass socket
		HANDLE clientThreadHandle = CreateThread(NULL, 0, StartListenThread, new InstanceSocketStruct(this, (LPVOID)clientSocket), 0, NULL);
	}

	// shutdown winsock
	closesocket(listenSocket);
	WSACleanup();
}

GameServer::~GameServer()
{

}

DWORD WINAPI GameServer::StartListenThread(void* s)
{
	InstanceSocketStruct* st = (InstanceSocketStruct*)s;
	st->instance->ListenThread(st->socket);
}

DWORD WINAPI GameServer::ListenThread(LPVOID socket)
{
	// retrieve passed socket
	SOCKET clientSocket = (SOCKET)socket;

	// listen for data
	while (true)
	{
		char length_byte[1];
		recv(clientSocket, length_byte, 1, 0); // read length byte
		Sleep(10);

		char packetID_byte[1];
		recv(clientSocket, packetID_byte, 1, 0); // read packetID byte
		Sleep(10);

		int data_length = length_byte[0];
		int packetID = packetID_byte[0];

		std::vector<char> data_bytes(data_length);
		recv(clientSocket, data_bytes.data(), data_length, 0); // read data
		Sleep(10);

		std::string data = std::string(data_bytes.begin(), data_bytes.end());

		
	}
}

Once this error has been resolved, I want to store the data about the client connection in a ClientConnection instance. Is the best way to do this by moving the thread creation code into a ClientConnection constructor, or simply keep it the way it is and pass HANDLE clientThreadHandle so the thread can be aborted later?

This post has been edited by Rixterz: 05 December 2017 - 05:05 PM

[Skydiver]

Thanks for posting the code, now what about the exact error messages and warnings. You can select all the error messages in the Error pane and copy them and paste here.

In the code that you just posted, you are not returning anything from either of the functions.