5Mixer / mphx

A little library to let you make multiplayer games easily with Haxe. No longer maintained and better options are available.
MIT License
125 stars 15 forks source link

Invalid Field Access / Long messages break sockets and serialization. #45

Open troyedwardsjr opened 8 years ago

troyedwardsjr commented 8 years ago

When running on Flash and Neko after sending enough events quickly enough, I get this error and it doesn't receive the event:

Connection.hx:131: CRITICAL - can't use data : oy4:dataoy5:recon Connection.hx:132: because : Invalid char at position 17 Connection.hx:131: CRITICAL - can't use data : i118y9:timestampd1471332997000y13:moveDirectiony4:leftgy1:ty13:Player%20Moveg Connection.hx:132: because : Invalid field access : t (event key itself?)

Connection.hx:131: CRITICAL - can't use data : oy1:ty13:Player%20Movey4:dataoy9:timestampd1471331618407y5:reconi199 Connection.hx:132: because : Invalid object Connection.hx:131: CRITICAL - can't use data : 6y13:moveDirectiony5:rightgg Connection.hx:132: because : Invalid char 6 at position 0

Connection.hx:131: CRITICAL - can't use data : oy1:ty13:Player%20Movey4:dataoy9:timestampd1471331618123y5:reconi1979y13:moveD Connection.hx:132: because : Invalid string length Connection.hx:131: CRITICAL - can't use data : irectiony5:rightgg Connection.hx:132: because : Invalid field access : t

Connection.hx:131: CRITICAL - can't use data : oy1:ty13:Player%20Movey4:dataoy9:timestampd1471331617874y5:reconi1964y13:moveDirectiony5 Connection.hx:132: because : Invalid string length Connection.hx:131: CRITICAL - can't use data : :rightgg Connection.hx:132: because : Invalid char : at position 0

5Mixer commented 8 years ago

Hmm, right. The "Can't use data" is the serialisation method trying to understand oy1:ty13:Player..... It often say's that t has invalid field access, and I'm pretty sure this is just because the received message is somehow being corrupted, somewhere along the path, or, the message is being fully received.

I tried quickly to replicate the issue by making a for loop that sends 200 messages at once, but, it just worked. At 1000 it was overloading server memory, and the server gracefully kicked the client.

Would you mind providing some extra details on the 'frequency' of these messages, where the error is happening (you say flash and neko, are you using another library, for example haxeflixel, server/client, etc), how many clients you have connected, and how you are using the library (screenshot/github).

Thanks!

troyedwardsjr commented 8 years ago

It's being updated every frame on a keypress event so potentially up to 60 times per second. I'm using Flash / Neko, and HaxeFlixel which uses OpenFL so it's update loop is listening to OpenFL's ENTER_FRAME event listener most likely, being called 60 times per second. It happens regardless of how many clients are connected, normally I test with two clients connected.

This is the data being sent:

var reconIterator:Int = 0;

override public function update (elapsed:Float) {
    super.update(elapsed);

    if (FlxG.keys.pressed.UP) {

        reconIterator += 1;
        clientSocket.send("Player Move", {
            moveDirection: "up",
            recon: reconIterator,
            timestamp: Date.now().getTime()
        });
    }
}
5Mixer commented 8 years ago

I should have asked this earlier, does the movement example work? It does essentially what is described hear, although, from memory, at a lower rate. I'm also considering that perhaps that date is messing things up in serialisation.

yannsucc commented 8 years ago

Perhaps join with issue #30. I resolved it by created a reader/writer socket class and a message manager for the client and server.

Imagine a message of 2048 bytes. I split it into 2 messages of 1024 bytes with this scheme: i[identifier]p[part]t[totalPart]s[MessSize]m[my_splitted_message]

The client serializes the structure (event + data) into a String and splits the data according to the scheme. The server uses the reader to parse the scheme and store the message-part into a message manager. When the message is ready to be rebuilt, the full String is unserialized and passed to the event manager (like it is now). This logic applies the same on both the client and server.

If a split message cannot be read for any reason, a full message cannot be rebuilt, and the message is lost. I suppose there is no way to know if the full message will be received (maybe need add a check of 20s maybe, not sure).

To make this, I attached some "patched classes". And overwrote mphx.FlashConnection, mphx.FlashServer and mphx.FlashClient

It's a work in progress, and some code will need to be refactored, but so far it's work perfectly.

⚠️ When you write an Int into a haxe.socket, the default endian is LITTLE_ENDIAN, however flash socket is by default in BIG_ENDIAN, which is incompatible. ⚠️

To read, it must be patched as follows:

    private function patchReadInt32(sock : NetSock) : Int
    {
        #if flash
        sock.socket.endian = flash.utils.Endian.LITTLE_ENDIAN;
        var value = sock.socket.readInt();
        sock.socket.endian = flash.utils.Endian.BIG_ENDIAN;
        return value;
        #else
        return sock.socket.input.readInt32();
        #end    
    }

I hope this "patched solution" help you to resolve your problem

readerwriter.zip

troyedwardsjr commented 8 years ago

@5Mixer:

Yeah the movement example works. All of the examples you have in the example folder works. There were some errors on the console, I think I sent you a screenshot of it on Twitter yesterday, but it didn't prevent it from being fully functional.

But the movement example is sending x and y values stored on the client and updating the server with it, and it isn't server authoritative. It's very easily susceptible to speed hacking/warp hacking, among other things, through code injection. That's fine for a coop game meant to be played with friends or any other game where hacking doesn't really make any difference, but not for any remotely competitive multiplayer game. It has to be server authoritative or it's a novice hacker's paradise.

You might already know about these, they're really good articles by Valve most people reference for the authoritative server model that most modern games use if you're interested in reading more about it: https://developer.valvesoftware.com/wiki/Source_Multiplayer_Networking https://developer.valvesoftware.com/wiki/Latency_Compensating_Methods_in_Client/Server_In-game_Protocol_Design_and_Optimization

As far as the date goes, I believe I tested it without the date/timestamp and still go the error sometimes, not 100% sure if I did, don't remember. I can try again, comparing before / after yannsucc's solution.

@yannsucc:

Thanks! I'll try it out and post the results on this issue when I get the time.

yannsucc commented 8 years ago

No problem. Just note that my solution will slow down the server a little (not significantly, but I haven't tested it with many clients). Ask if you have any questions, but I think the package explains it all. Only change the import/package on the top of each class, and don't forget to use var serv = new ImprovedFlashServer() instead of new Server(); (same client side)

troyedwardsjr commented 8 years ago

@yannsucc:

Is there a file missing from the zip? Not sure if it's necessary but it's referencing monitoring.L

comTools/SocketRW.hx:6: characters 7-19 : Type not found : monitoring.L

yannsucc commented 8 years ago

replace all L.("blabla","foo") by trace("blabla"). it's only a personnal log system sorry

you can delete it too if you want but you can miss some important log

troyedwardsjr commented 8 years ago

@yannsucc:

Cool, it fixed the issue!

The first time I tried it, there was an error at some point but it's too far back in my console window now and I forgot to copy it, so hopefully it was either a fluke or I can replicate it later.

But the second time I tried it and ran it for a long time with no desynchs / dropped requests and it solved the issue. Works great! There are definitely performance issues, but as you said it hasn't been optimized yet.

Also got this on the client but it didn't crash the client or drop any requests:

SocketRW.hx:256: Error: Error #2030: End of file was encountered.

I'll continue to test it. Thanks again.

troyedwardsjr commented 8 years ago

@yannsucc:

Can this solution be carried over to the other targets? Neko, HTML5, ect.

troyedwardsjr commented 8 years ago

@yannsucc:

After testing more I managed to get the error again:

FlashConnection.hx:153: CRITICAL - can't use data : oy1:ty13:Player%20Movey4:datao FlashConnection.hx:154: because : Invalid char at position 30 ImprovedFlashServer.hx:141: Socket read error : Not an header,server ImprovedFlashServer.hx:141: Socket read error : Not an header,server ImprovedFlashServer.hx:141: Socket read error : Not an header,server ImprovedFlashServer.hx:141: Socket read error : Not an header,server ImprovedFlashServer.hx:141: Socket read error : Invalid header,server ImprovedFlashServer.hx:141: Socket read error : Not an header,server

yannsucc commented 8 years ago

Not sure. I use this for a specific project with cpp/neko target and flash client, but except for some flash specific code, I suppose it should work for other targets.

SocketRW.hx:256: Error: Error #2030: End of file was encountered.

hum, I forget some error management code, add this :

#if flash
else if (Std.is(e, flash.errors.EOFError))
{
    //End of message. Not an error - This is still a connected, valid client.
}
#end

between line 251 and line 255 (after the if haxe.io.Error.Blocked) That should resolve the problem.

yannsucc commented 8 years ago

Two step

First :

FlashConnection.hx:153: CRITICAL - can't use data : oy1:ty13:Player%20Movey4:datao FlashConnection.hx:154: because : Invalid char at position 30

=> server send a raw data or fail to deserialise (don't know why)

Second :

ImprovedFlashServer.hx:141: Socket read error : Not an header,server ImprovedFlashServer.hx:141: Socket read error : Not an header,server

Because of step 1, the readerSW read the socket until find the first caractère of a valid header ("i")

yannsucc commented 8 years ago

Nvm, forget this post.

troyedwardsjr commented 8 years ago

@yannsucc:

Right now I'm instantiating it like this.

import net.patch.ImproveFlashClient; clientSocket = new ImproveFlashClient(GameData.ip, GameData.port); clientSocket.connect();

Am I supposed to be changing the Client.hx from

elseif flash

import mphx.client.impl.TcpFlashClient; typedef Client = TcpFlashClient;

else

to

elseif flash

import net.patch.ImproveFlashClient; typedef Client = ImproveFlashClient;

else

instead or something?

troyedwardsjr commented 8 years ago

@yannsucc:

Also this is a flash-only target implementation right? Because if I try to compile to neko it gives me this error. I see that the class extends TcpFlashClient:

source/net/patch/ImproveFlashClient.hx:4: characters 7-38 : Invalid package : mphx.client.impl should be empty

source/net/patch/ImproveFlashClient.hx:11: lines 11-74 : Type not found : TcpFlashClient

troyedwardsjr commented 8 years ago

On the server-side I'm instantiating it like this by the way:

var test = new comTools.SocketRW(); var s = new serv.ImprovedFlashServer(ip, 8000, test);

and in ImprovedFlashServer.hx it has:

import serv.ImprovedFlashConnection; this.connectionTemplate = new ImprovedFlashConnection(hostname, port, m_socketRW);

So I don't know why FlashConnection.hx is showing up in that error. It works for a while then that error will randomly, rarely pop up.

I might reorganize the package names later to patch it correctly so I can be 100% sure. I didn't change the package names before because I was just testing it quickly.

yannsucc commented 8 years ago

Yep, this fix is only for flash client target. i hav'nt time to make it multi target.

yep is strange, maybe there are an error on writing when message <1024o on socketRW. but your implementations seems OK.

As i said, is WIP and i have not this error in my project. Event if i send many log at the same frames on the socket.

Very strange. But i don't know how to fix it now, and i have not time for this at the moment. sorry. If you find it, feel free to post your fix :D