Flone-dnb / FBugReporter

Easily add feedback and/or bug reporting functionality to your Godot game.
MIT License
63 stars 4 forks source link

Use a webframework instead of raw TCP #7

Open LoipesMas opened 2 years ago

LoipesMas commented 2 years ago

Looking at the server code, it seems that a big part of it (handling connections, encryption, reading from sockets) could be removed if you would use a web framework (such as rocket or actix). This could improve code readability (less code, better structured), security/stability (replacing custom implementations with tested ones) and perhaps even performance. Also using a more standard way would make it easier to grasp what's going on and expand the functionalities.

What do you think? Or is there a reason why you chose to not use a web framework?

Flone-dnb commented 2 years ago

Initially I started using raw sockets because I've written code in the past using raw sockets.

Eventually we should probably switch to something like actix but right now I don't see a good reason for this switch (because it would require a lot of work). Current networking code is... done. Network messaging is implemented in the 'shared' crate and right now every crate of this project just uses functions like send_message or receive_message and adding new messages is as simple as adding a new entry to an enum. I haven't encountered any critical issues with current networking so I don't see a reason for this switch. The only issue that I see right now is that we are dedicated a whole thread for each connection (which is obviously not a good thing).

I had some experience with actix for web servers in the past, should we use something like websockets or something else would be more appropriate here?

LoipesMas commented 2 years ago

For a (relatively) simple performance improvement we could replace std with tokio which is async.

I'd argue that it's better to rewrite it sooner than later, because you have less code to rewrite and you won't write code that will be rewritten later. But if you don't plan on changing/extending current network code, then this can wait.

Also I was thinking about a HTTP server, especially for reporting (because there you make a single request once in a while). For clients HTTP server should work too, because you don't really have a stream of data. But websockets should be fine too.

Flone-dnb commented 2 years ago

I'd argue that it's better to rewrite it sooner than later, because you have less code to rewrite and you won't write code that will be rewritten later.

Agreed, but I don't want to change something that works. Moreover even right now it would require a lot of time to rewrite this (the time that I don't and probably won't have in the future).

As I've said, eventually we should totally do this. But right now we have more important stuff to implement.

So I'm putting this issue on hold, until somebody will start implementing this.

LoipesMas commented 2 years ago

Sure, that's reasonable.

Flone-dnb commented 1 year ago

@LoipesMas I got interested in Salvo, do you think this can fit our needs? We can then remove the client application to just have a web interface that the server provides.

Flone-dnb commented 1 year ago

So I just gave this a quick though and although I know that there are some great pros for using a web framework but wouldn't it require our users to spend money on SSL certificates? I only know some base information on the web topic so help me out here.

I'm pretty sure most of our users are indie developers, who probably don't want to spend much money/time on setting up certificates, remembering to renew them and just deal with all this certificate stuff (since probably they will already spend a little money on a cloud Linux machine to host the server). Our current solution requires almost no effort to set everything up and it will be secured (relatively) enough. I mean this is not some bank management application, bug reports don't hold any sensitive information except maybe for the email addresses (if provided by the reporter).

Don't get me wrong, I would actually like to switch to some library that handles networking but only if it won't require our users to do some complicated setups or spend more money so help me out here.

P.S. Moreover, maybe #3 will be enough for most of our users since quite a lot of people were asking for it and we won't even need to care about doing all networking ourselves.

gedw99 commented 1 year ago

hey @Flone-dnb

Maybe NATS fits the bill here... Its tcp, has create rust and golang support. scales like nuts. have built in security if you want it. It also supports web sockets as maybe thats needed.

tons of game developers use nats to btw.

https://github.com/nats-io/nats-server

golang client: https://github.com/nats-io/nats.go

rust client: https://github.com/nats-io/nats.rs

the NATS server can store the screenshots and data using nats objj store and kv store if you just want to keep things simple.

using the api is really easy i find from rust and golang.

LoipesMas commented 1 year ago

@Flone-dnb with certbot, you can set up free and automatic certificates for your https server. This shouldn't be much harder than setting up the server itself. But having a certificate wouldn't really be a requirement. As you said, it's not really sensitive information and the requests could be sent over http (without the s).

So considering how simple and standard using https would be, there's not a lot of reasons not to. But on the other hand, it's also not a high-priority thing.

Flone-dnb commented 1 year ago

@gedw99 You are right, NATS can actually be one of the solutions. Although I haven't used NATS myself, I know that some C++ game projects from my job use NATS. Unfortunately, I think NATS will be overkill for this project. I already wrote a very simple API for handling encrypted TCP messaging in the shared crate (including file transfer), it works fine and seems to not have any critical issues (although I think our file transfer or the fact that it does not use async will slow us down) now I'm just looking for a proper "battle-tested" simple / lightweight library for handing low-level TCP stuff for me.

@LoipesMas Hey thanks for the info, I didn't knew about certbot.

As you said, it's not really sensitive information and the requests could be sent over http (without the s).

I'm glad you can agree on this, although when I said:

I mean this is not some bank management application, bug reports don't hold any sensitive information

I meant that at least some (poor) encryption is required (we don't need super secured stuff here), moreover, I don't want to just throw away encryption that we currently have. I mean it's not perfect and probably has some security issues, but hey it's better that nothing. Additionally, although reports might not have much sensitive information, client login/passwords do have sensitive information. Imagine you are a developer and you want to check the report list, you connect to the server by sending there your login and password hash - I think this is sensitive information, sending this over HTTP is probably a bad idea.

So I think that we should at least stick with the web framework + certbot solution. Maybe we can detect if the server (built in the release mode) is started in the HTTP mode and just throw an error and don't start at all to prevent the users from shooting themselves in the foot. Yeah, this might work.

So OK, let's say our 2.0 goal is to:

This will most likely happen in the next year (because I don't have time for this now and this is not a high priority). But don't think there won't be any updates this year. I already updated the project to support Godot 4 and I plan to try to add Github Issues support this/next week (see dev branch).

Any thumbs up/down on this?

LoipesMas commented 1 year ago

Web framework + certbot sounds good! Haven't used salvo myself, but it should fit our needs.

Somewhere down the line you could consider selling servers as a service (setting up, maintenance, etc.), for those users who don't want to bother with that, in order to fund further development. But that's just a thought for now.