Packet Handler

[ebf34e12952930cf]

instead of the huge if-else list in clientless, would something like this be better?

[toddw123]

Eh the if-else will eventually be moved to a function that is part of the Client class, as i plan to make it so you could create more then 1 client (bot) at once. That way the Client class will then have direct access to all the packets coming in. Id rather not have a bunch of "packet handler" classes that i then have to include in the Client class as It just seems like having handlers for each packet is unnecessary imo.

Now, i have made most of the packet classes function both inbound and outbound, and thus this project could be converted into a Proxy if it was modified a little bit. If this project was a Proxy then i would 100% agree that having a packet handler class for each packet type would be a good idea.

[ebf34e12952930cf]

aight

[toddw123]

Im reopening this one because i think i want to go in this direction now, and i could use a little help putting together the handler and such.

After looking over lots of different code examples and revisiting jOryx and KRelay's code, i think having a packetHandler type of functionality would be better in the long run.

The part im not sure of yet is threads. I want to make it so each new packet that comes in will kick off a thread that handles the packet. My concern is reading/writing data that is being used. Like when a client first connects and there are 30-40 update packets sent. Thats 30-40 threads all attempting to add info from the update packet to various variables in the client class. The solution of course is to use either a mutex or singleton or something similar that will prevent anything from trying to read from a variable that is being written to. The other option is to make it so its not a thread, but that doesnt seem to have any advantage over the current code that i can see.

I also want to look more into non-blocking sockets. I have only ever read tutorials on non-blocking and looked at examples, ive never done more then compile some example code and watched it work. Because of the current setup being blocking, i think its throwing times off. I have been playing around with my walking speed and i was trying time the time between newtick packets so i could work it into the equation but it constantly told me the same exact time as the previous. My guess is this is due to so many packets being received that its processing them 1 by 1. This is what has made me want to switch over to the handler/callback approach and put them in threads that way i can read packets as soon as they come in without causing them to build up or anything.

@BlackRayquaza im tagging you since i cant make this topic appear at the top or anything like that and i would like to see what you have to say here. Also, one thing i dont quite understand about the code/image you posted in the first post here, why do you have the unique class inherit the PacketHandler<> class? It seems like the PacketHandler class is all you need, especially since its a template class, there shouldnt be any need to create a new class to inherit it as far as i can tell. But let me know, im interested to hear what you can say on this and love to have some help creating it.

[Zeroeh]

1. Are non-blocking sockets the same thing as asynchronous sockets? (used to .NET wordings so idk if they are the same but it sounds like it)
2. I noticed that when I have the bots follow my player, the bots will eventually start to move one at a time.
3. I agree about the image, it's weird how you would implement a whole new class for each packet. The PacketHandler<> should have its own functions.

[toddw123]

1. umm sorta. This is how its generally explained: with blocking sockets (which are the basic type of sockets unless you change it) when you call recv() it will wait until it gets data or timesout. So everything else is just sitting there while it waits for it to return a value. Non-blocking sockets will call the function, if nothing is there to read it will move on and return a value saying there was nothing to read.

2. Depending on how many bots you have running, that could just be network related. But also might be because of the blocking socket. lots of things could cause it really.

3. well the idea of the PacketHandler in my opinion is to just call a callback. Although i guess you could pull all the packet handling code in unique classes to split it up. Im still looking into this and trying to find something that i like.

[toddw123]

I could even just avoid the PacketHandler class and just do this: std::unordered_map<PacketType, std::function<void(Packet)>> packetHandlers;

That would allow me to add a callback to a function based on the packet type. Kind of like k-relay where you have a function that gets called anytime a packet comes in. So for any packet i want to use, i could just do something like: packetHandlers[PacketType::UPDATE] = onUpdate;

and then i would have a function in the client class void onUpdate(Packet p);

theres more involved then just this obviously, but thats the general idea. im going to start putting this together for now to see if i like how it works or not.

[toddw123]

It looks like from some quick testing that i can use the basic example i posted above.

In the Client class i added this to the protected members: std::unordered_map<PacketType, std::function<void(Packet)>> packetHandlers;

Then in the start function of Client i can set up whatever hooks i want (might make a function to have it look prettier): packetHandlers[PacketType::UPDATE] = std::bind(&Client::onUpdate, this, std::placeholders::_1);

And finally, i just have a basic function for testing at the moment: void Client::onUpdate(Packet p) { printf("%s called onUpdate!\n", this->guid.c_str()); }

Then in the recvThread, anytime i get an update packet i call the hook by simply going: packetHandlers[PacketType::UPDATE](pack);

I will eventually change that so it will look something like: packetHandlers[head.id](pack) that way i dont have a bunch of lines where i call the exact hook.

Now i just need to get a mutex/singleton set up and put the call into a thread and this should be good.

Although im still interested to hear from BlackRayquaza on this whole thing and how you would implement the pseudo-code you posted

edit: you can view this super basic setup in the new branch is just created, packet-handler-test. I didnt do anything more then convert the giant if-statement into functions. I still havent added threading for the functions or mutex/singleton yet. But it does work. It does exactly was it was before, except now its broken out into functions. Pretty neat. Next step is to thread it so it has better speed.

[ebf34e12952930cf]

i actually had most of it working, just couldnt figure out the unordered_map part, the <template T> (or whatever it is in C++) is so you can use the actual Packet Types, and not just Packet

[toddw123]

Gotcha. Yeah i like the look of the code you had up top, but the more i looked into it the more it didnt seem to be logical to have a new class for each and every packet. If you look at the example i put together i also avoided the base class of PacketHandler, which i was debating if it was needed or not. On the one hand, it looks better to have a base "handler" class to hold all the handles...but that just opens up more problems in my eyes. Id still be interested to see a put together source going off the style you presented, but after looking at example after example after example today i dont think having it that separated is necessary. :)

[Zeroeh]

I think having just one packet handler class with a callback to a function for each packet id would be perfect, as well as simple.

[toddw123]

If you can put together the basic code for that im all for it. But like i just said, after looking over examples and playing with code i cant see the reason for a PacketHandler class, all thats needed is the unordered_map and the functions to handle the packet. I cant see a good way to work an unneeded class in there, as much as the idea sounds good. Feel free to browse the new branch and look at how i currently have it implemented in client.cpp/client.h. If you can re-arrange that to use a PacketHandler class and it makes sense, i completely agree it would be good.

[ebf34e12952930cf]

can maybe move the functions to its own class to make Client less cluttered

[toddw123]

Anyone have a good idea/solution for thread safety? I was going to try and use std::mutex, but i need it to be unique to each Client. To add std::mutex to a class it has to be static. So that doesnt work...

I also looked at various singleton stuff but it seems a lot of people are against using singletons as they can be unreliable.

I just stumbled upon std::atomic and it sounds like its what i am looking for, but im not entirely sure yet on that one. I have never heard of it before so its completely new to me what this std::atomic actually is. So ill have to play around with it while still looking for an answer.

To cover what im looking for again; i plan to kick off the different packet handling functions as their own thread. The only problem with this is that there might be multiple threads trying to write to a variable related to the update packet, or i might be trying to read from a variable that is getting updated in another thread. Crap like that. I tested without a mutex/singleton/lock and just 1 client and it crashed after about 10 seconds due to this i believe.

In the past when i have done similar things like this i would end up creating a get/set function for every variable (yeah i know this is usually seen as the route to go anyways) and then in the get/set functions i had a mutex to prevent simultaneously reading/writing. But the last time i used mutexes i used a windows specific version, and i dont want to do that this time. I would prefer to keep it within the realm of standard c++, that way down the line this can be cross-compiled. The only part windows specific right now is winsock, but that can be fixed to be cross-compatible easily. And i think the getTime function. But this is a different topic altogether lol.

So, anyone have a good idea here?

[VoOoLoX]

I was going to suggest a method with getters and setters but that doesn't seems to be what you'd want, and since I'm not really into c++ i can't suggest anything specific but some sort of queue maybe?

[toddw123]

well like i said, i like using get/set to handle it, but the doesnt solve the problem of thread safety. If one thread calls set() while another calls get() it will either cause problems or crash the program. A queue doesnt make too much sense in this context. The idea is i need some kind of mutex to lock the sections im reading/writing variables. Look up mutex and you will understand more about what im talking about :)

[VoOoLoX]

I looked into a mutex and it seems to be much like the locks in c# anyway, is it possible to have a queue of get and set calls and just execute them in the linear order (FIFO i guess?) ?

EDIT: I guess that wouldn't work with multiple threads, would it?

[toddw123]

that would be a lot of work for a not so easy work around, having to figure out how to put together a queue that hold different functions with different parameters isnt as easy as it might be in C#. And its also susceptible to the same issue its trying to prevent. If two threads attempt to add something to the queue at the same time, it can cause issues. Your idea would prevent the functions from touching the variables at the same time, yes, but the functions that add to the queue is still not protected.

A mutex or variation of mutex is the solution. The problem im having right now, that doesnt make any sense to me from what i have been looking at online and remember doing in the past, is that i cant put std::mutex in the client class unless its static. If i dont define it as static then it throws errors. If i define it as static, then that means its the same mutex through ALL clients, and i dont want that. I want a unique mutex for each client. Possibly even multiple mutexes per client. I checked out the std::atomic class and that is also fairly similar to what im looking for, but it too throws errors when i put it into the client class. Its baffling.

edit: it looks like i might be able to do something like std::vector<std::mutex> mutexes([size]) where [size] is a hardcoded size. According to everything i could find online about using an array/vector/map of mutexes, it has to be a set size. So i can create a vector of them, then in the start() method i can just assign an id to each client since the client::start() is called one after the other. time to try this out.

edit2: the more im playing around with threading the packet handlers the more it seems like a bad idea. even if i can get mutexes/thread safe variables working...it seems to be a mess. The better idea, and this is what ive done in the past when making bots for games, is to just have a thread constantly reading in packets and passing them to a thread safe queue, then another thread reads from that queue and parses them. This way the parsing of the packets doesnt prevent new packets from coming in etc etc etc. The only problem is, it can cause the outgoing packets to lag behind the incoming packets a little bit since it has to go through the queue and everything. Hmm.

edit3: okay...so putting inbound packets into a queue might not work either. It works for a little bit, then it eventually disconnects. My guess is the fact that the server isnt getting responses for each packet before it sends another. Its just sending packets non-stop and is basically getting packets randomly from the client. So....looks like its back to a direct recv -> processes -> reply. Which makes the change in switching to packet handler functions purely aesthetic it seems.

[Zeroeh]

I'm still getting used to c++ but this is certainly something I'd like to try working on sometime. I'm aiming for near 100% efficiency ;)

Anyways, I'm almost certain that what you're looking for is atomics. You're just adding it to the client class is where it's causing issues?

[toddw123]

I tried out atomics earlier when I mentioned it and it wouldn't compile unless it was outside the class or static (like the mutex).

But, as I updated the previous post to say, threading the handlers or even putting the packets into a queue and then handling them won't work. If the game was UDP instead of TCP a queue for the inbound packets might work, but since it's TCP the server can pretty much know that the client isn't responding right away/fast enough. The packets have to be handled as soon as they come in otherwise the server will kick the client. It's pretty dumb but that's what my testing has shown. I tried putting the packets into a queue as they came in and had a separate thread running that would run through the queue and pass the packet to the handler function, but that delay in a response produced errors. If I ran the client with the queue I would get kicked after about 15 seconds. If I handle the packets as they come in, I don't get kicked at all. So oh well.

So theres no need for a mutex/atomic variables anymore since the packet handling can't be threaded (unless someone can put together a way to thread the handlers that doesn't cause the server to kick the client after a few seconds).