Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design new network protocol to be used for communication between server and client #70

Open
pehala opened this issue Oct 19, 2020 · 26 comments
Labels
networking Issues related to the server client network communication

Comments

@pehala
Copy link
Collaborator

pehala commented Oct 19, 2020

Currently, the server-client communication is based on text messages separated by tab (e.g starttrack \t trackname...), processed by one big if statement across multiple methods. It is a mess and sadly totally unmaintanable since we cannot read most of it and do not understand how it works at all.

I would like to start designing a new proper protocol which would include all of the messages that are currently processed and that would allow extending it to for example enable user roles or even check version.

To do that I think we should have a separate branch because this is an effort that will take a long time a lot of work.
WDYT @PhilippvK ?

@pehala pehala added the networking Issues related to the server client network communication label Oct 19, 2020
@PhilippvK
Copy link
Owner

@pehala Great ideas! But definitely a huge workload which would be better done on a new branch. Feel free to work directly on this repository instead of your fork if its easier for you, I can give you write permissions if not already granted.

@PhilippvK
Copy link
Owner

I am not sure if it's really suited here but the server-client communications needs some sort of protection against conflics/race conditions when multiple messages come in at the same time. (f.e. synchronized blocks or something at a higher abstraction level)

@pehala
Copy link
Collaborator Author

pehala commented Oct 19, 2020

No need, I would still like someone to review the code, to make sure it is correct. I would also like to discuss this with you in more detail to make sure the design is done right and if that would be possible I would like to ask you to help me with the mapping of existing "endpoints" from client because the code is totally a mess.

@pehala
Copy link
Collaborator Author

pehala commented Oct 19, 2020

I am not sure if it's really suited here but the server-client communications needs some sort of protection against conflics/race conditions when multiple messages come in at the same time. (f.e. synchronized blocks or something at a higher abstraction level)

I definitely agree, that is taken care of netty (if we would use netty correctly) so it shouldn't be that hard.
Also, we should also write networking tests to have at least some confidence in the new networking.

EDIT: Oh and we have to use special collections (CopyOnWriteArrayList etc) to make sure that it works with parallel access.

@PhilippvK
Copy link
Owner

No need, I would still like someone to review the code, to make sure it is correct. I would also like to discuss this with you in more detail to make sure the design is done right and if that would be possible I would like to ask you to help me with the mapping of existing "endpoints" from client because the code is totally a mess.

Fine! I do not really see myself writing any "new" high level Java code as my Java skills are way behind yours and I am tbh not really into Java. Reverse Engineering the current protocol stack and help design a new concept would be a more favorable tasks If I am able to invest time in it.

@capsload2
Copy link

Is there any chance we can add more than 4 players?

@PhilippvK
Copy link
Owner

PhilippvK commented Oct 26, 2020

@capsload2 See #71!

@PhilippvK
Copy link
Owner

@pehala I digged a bit through the codebase and got a bit more familiar with some endpoints and how the connection works.

I have a few open questions to you:

  • Do you want to use just Netty Pipelines with Decoders+Handlers on both Client- and Server-Side or change to some higher level library?
  • Should the String/Delimiter based protocol be replaced with serialized (binary) Objects or something else?
  • What are specific endpoints which we still have to figure out?

@pehala
Copy link
Collaborator Author

pehala commented Nov 2, 2020

  • Yes, definitely I will migrate to Netty 4.x but other than that it will still be Netty.
  • Yes
  • I actually dont know any of them properly, so if you could maybe create some document with all you could find. We can then discuss individual endpoints if they are needed or not etc. If you could also try to figure out what parameters these endpoints need it would be perfect.

@pehala
Copy link
Collaborator Author

pehala commented Nov 2, 2020

Maybe adding NETWORK.md with all endpoints could be a good idea, we will document it in the process which is always a good idea

@PhilippvK
Copy link
Owner

@pehala Have you already found this document?

https://github.com/PhilippvK/playforia-minigolf/blob/master/doc/servertoclient.txt

Its not as helpful as I wished but it contains at least a subset of endpoints...

@pehala
Copy link
Collaborator Author

pehala commented Nov 2, 2020

I did not, thanks!

@PhilippvK
Copy link
Owner

Do you think de-obfuscating the variable- and function-names in the client-side connection code would be a good initial/intermediate step or would it become obsolete when rewriting the protocol anyway?

We could f.e. migrate to netty 4 with the current protocol before we replace it with a new one.

I also found out that the protocol already supports server-client version verification in some way, but it is not really used at the moment :D

@pehala
Copy link
Collaborator Author

pehala commented Nov 2, 2020

Do you think de-obfuscating the variable- and function-names in the client-side connection code would be a good initial/intermediate step or would it become obsolete when rewriting the protocol anyway?

Yes definitely

We could f.e. migrate to netty 4 with the current protocol before we replace it with a new one.

Probably not, it would require time to replace deprecated components which I think is not very useful since we want to scrap it all together.

I also found out that the protocol already supports server-client version verification in some way, but it is not really used at the moment :D

Lol, nice :D I would probably not enable it or use it right now, I think we can wait with this for the new one. I would rather have polished feature later than PoC early.

@pehala
Copy link
Collaborator Author

pehala commented Jan 11, 2021

After spontaneous reserach I think we can base our protocol on top of WebSockets. Since it is using HTTP connection, we would be able to hide our server behind HTTP proxy, which is currently not possible (I have tried).

@PhilippvK
Copy link
Owner

Wow, I actually hd the same thoughs just a few days ago! I tried Porting the Client Java Applet to HTML5+JS via the CheerpJ Compiler. The problem is that Browsers can only communicate via HTTP/HTTPS/WebSockets which does not work at all with the Netty TCP Sockets. Unfortunately CheerpJ does only support native Java8 Components which means that java.net.http.WebSocket won't work, yet. As this means that the only remaining option right now to get CheerpJ working, would be replacing the Protocol with a HTTP REST API which sound terrible for a game engine...

Anyway, WebSockets would solve this issue if Java11 is supported by the tool in the future. Great Idea! Would you like to stick with Netty for realizing this approach?

@pehala
Copy link
Collaborator Author

pehala commented Jan 11, 2021

Yes I would like to stick with Netty. Quick research showed that they are already some implementation of websockets with netty, even tho I didn't test them yet. I plan to test this with a prototype once I have some free time left (=most likely my exam period over)

@pehala
Copy link
Collaborator Author

pehala commented Jan 11, 2021

Quick note: it might be possible to port existing TCP netty application to WebSockets by changing handler, which takes care of the actual protocol mapping.

@PhilippvK
Copy link
Owner

Good points!

But as far as I know only the server-side of the communication uses Netty at the moment. Client uses plain sockets. I assume that we should possibly port the client to use netty as well first.

@PhilippvK
Copy link
Owner

Similar situation for me. I hope that I manage to invest some time in the project in March because exams are more important at the moment.

@pehala
Copy link
Collaborator Author

pehala commented Jan 11, 2021

Good points!

But as far as I know only the server-side of the communication uses Netty at the moment. Client uses plain sockets. I assume that we should possibly port the client to use netty as well first.

Yep that will definitely be the first pain point

@PhilippvK
Copy link
Owner

It might be not really related to the Issue but I managed to get the Client Applet running using CheerpJ to compile from JAR to HTML5+JS. As mentioned before I had to write a custom Adapter to java.net.Socket in JS to use Websockets on Port 80. The Server can be used without modifications using websockify 80 :4242. It should work with every desktop browser without any Addons and could be hosted f.e. on GitHub pages but I am not sure if I am going to deploy it because the code is quite a mess right now.

Screenshot 2021-01-18 at 05 50 13

@pehala
Copy link
Collaborator Author

pehala commented Jan 18, 2021

Nice! I would like to still like to properly rewrite client networking since we have the option and time, but this is awesome for all other minigames.

@pehala
Copy link
Collaborator Author

pehala commented Aug 28, 2021

I finally started with rewrite on top of latest Netty (4.1). Suprisingly, the pure socket client is not that hardly connected to the rest of the applet so I was able to replace it with my own.

I choose to design the communication from scratch because it seemed a lot of network messages currently in use.

I have currently a small PoC which so far is integrated as far as lobby select. Due to the nature of the rewrite (absolutely everything network related :D) it will be a very big PR, @PhilippvK would you like me to create draft PR which will be continuously updated so we can discuss changes early or do you want me to submit once I finish everything?

@pehala
Copy link
Collaborator Author

pehala commented Aug 28, 2021

Also regarding the websockets, since we are using Netty, it is very easy to switch underlying protocol to pure websockets if we want to also embed it into HTML

@PhilippvK
Copy link
Owner

Due to the nature of the rewrite (absolutely everything network related :D) it will be a very big PR, @PhilippvK would you like me to create draft PR which will be continuously updated so we can discuss changes early or do you want me to submit once I finish everything?

BBoth is fine to me. But I would prefer if I can either check the current state out via the PR or by just using your fork of the repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking Issues related to the server client network communication
Projects
None yet
Development

No branches or pull requests

3 participants