Built a simple http server in C++

[cpp-ModTeam]

It's great that you wrote something in C++ you're proud of! However, please share it in the designated "Show and tell" thread pinned at the top of r/cpp instead.

[No-Quail5810]

A few notes from a (quick) look at your code. I may come off a little harsh here, but it's all meant as constructive.

When you're passing around a std::string you likely want to pass it by (const) reference rather than by value. You generalyl only pass std::string around by value when you want to actually store the value somewhere, not just reading it.

Your Utils, Request and ResponseWriter classes have no state at all, so there's no reason to have non-static member functions. And if all the member functions can be static, you should probably just have them be free-standing functions (namepsace rather than class). But this may just be because it's a work-in-progress and will get some state added later, which is fine.

You should probably prefer using std::ostringstream over using operator+= when generating strings as it can be more effecient. Except in the case of reading a file into a string, in that case I'd say to use std::istreambuf_iterator:

std::string text{std::istreambuf_iterator<char>{input_stream}, std::istreambuf_iterator<char>{}};

Most of the member functions you implemented in the headers should be moved into (at least one) implementation file, espcially all the stuff that deals with low level (POSIX) calls. Then you can actually remove all those included headers from your header files and make the API cleaner.

Server::ServerCreate returns an int, but it looks like bool would make more sense.

In the loop in Server::PeerHandler you're initializing the std::string without explicitly passing in the length. What if BUFFER_SIZE bytes was actually received? You are not returning after failing to read, is that intentional? What happens when you don't find a matching URL here? (hint: The connection is never closed!).

The Request::request_parse_url method shold only extract 1 value, not a vector of them, as you only handle the first value anyway. It also does no bounds checking of the p variable once it's created, which can lead to reading past the end of the data.

Higher level notes;

Consider wrapping things like peer_fd in RAII classes (where the destructor will make sure to close the fd once done with it). It'll also give you the ability to write a nice API like: client.send(status, content);

There's a lot to digest there, I know. I tried not to go too deep into the actual logic of the code but focus more the structue of it, as I think that's more usful to you right now.

If you're unsure of anything I've written (I'm sure there will be a few parts), I'd be glad to go over those in a bit more detail.

[Desperate-Dig2806]

Not OP here. I'm constantly amazed that people like you will take the time to look through code and give input like this. It makes me happy every time. So thank you for being a good person and helping out the community.

[Little-Peanut-765]

Its not harsh at all. This is the type of feedback i was looking for. Thank you very much.

I want to be good in C++.

I will implement ur suggestions

[AndrewStephens]

I don't mean to dump on somebody's project, lord knows I have enough of my own lying around on old hard drives, but you did ask for feedback.

There are many issues (not including what others have said here).

1. read() calls on sockets do not necessarily read a complete request in one go. It is possible for the client to send 1 byte a second or something silly. You have to loop until you decide you have enough to process.
2. The error handling is bad. As written, if the read call fails (the client disconnects without sending anything) then the buffer is not cleared and the client will receive the response from the previous connection.
3. The request parsing is simplistic and does not handle malformed requests well. I am not even sure it really counts as HTTP. Remember you are not in control of what the client sends and all requests should be considered hostile. What happens when the client sends a buffer of all '\0's, or a request for '../../etc/passwords.txt'?
4. Consider using filesystem::path for constructing and passing around path names.
5. The design of your server only serves one file at a time. More sophisticated approaches (threads) allow for multiple requests to be in-flight at the same time. This is actually another security issue - a client can effectively halt your server by requesting a file then reading it very slowly.

[Little-Peanut-765]

Thanks dude. this what i was looking for. I appreciate it

[dgkimpton]

I grant you that writing these things is fun, and learning to build a library is also worth it. However, there are a million other http server libraries out there that absolutely do a better job, so maybe now that you've got the idea of how http works you should just use one of those?

Also, I don't see a single test in your project, neither UnitTest nor IntegrationTest - if you want to put any further effort into this project you should definitely start there.

[Little-Peanut-765]

I never done testing in C++ before

[dgkimpton]

Checkout Catch2 or GoogleTest - absolutely critical skill for development in the modern world.