Uhhh.... Clean... Up.. Code..?

[highwayman]

I've been making this little thing(I guess you could technically call it a framework,but it's kinda too shitty for that.) that is supposed to set up a TCP Server for you.
You can start() that server which causes it to listen() and then make several threads. start() returns a bunch of threads.
All except for one of those threads are worker threads which take a CONN(a struct I made that just holds the file descriptor and address info of a single accept()ed connection. The address info is a list.) and pass the inner file descriptor and address to a handler function.
The last thread is running a function called fill_queue(), which (as the name suggests) adds to the queue of CONNs.

this is a lot.

I don't like how much it is.

I don't like having unnecessary lists of unused data.

I don't like having an unspecified arbitrary number of threads running at once.

I don't like this thing, so I'm currently trying to figure put how to reduce the amount of resources I am using as much as I can.


Any suggestions or criticisms would be absolutely lovely. thank you! :)

Here's the code.

Conn_Queue.hpp

 // Conn_Queue.hpp
#ifndef P_CONN_QUEUE_H
#define P_CONN_QUEUE_H
#include <condition_variable>
#include <mutex>
#include <queue>
#include <list>
#include <sys/socket.h>

class CONN_QUEUE {
  public:
  struct CONN {
    sockaddr_storage *sockaddress;
    int fd;
  };

  CONN_QUEUE() = default;

  void wait();

  void push(CONN connection);

  CONN pop();

  private:
  std::queue<CONN,std::list<CONN>> connections;

  std::condition_variable queue_ready;

  std::unique_lock<std::mutex> q_mtx;

};

namespace CONN_HANDLERS {
  void fill_queue(CONN_QUEUE* cq,int m_fd);

  void worker_thread(void (*operation)(int&,sockaddr_storage&), CONN_QUEUE* cq, bool auto_close);
}

#endif 

Conn_Queue.cpp

 // Conn_Queue.cpp
#include <mutex> // std::unique_lock
#include <condition_variable> // std::condition_variable
#include <list> // std::list
#include <queue> // std::queue 
#include <sys/socket.h> // sockaddr_storage, socklen_t, accept()
#include <unistd.h> // close()
#include "Conn_Queue.hpp"


void CONN_QUEUE::wait()   {  queue_ready.wait(q_mtx);  }

void CONN_QUEUE::push(CONN_QUEUE::CONN connection) {
  q_mtx.lock();
  connections.push(connection);
  q_mtx.unlock();
  queue_ready.notify_one();
}

CONN_QUEUE::CONN CONN_QUEUE::pop() {
  q_mtx.lock();
  CONN ret;
  if(!connections.empty()) {
    ret = connections.back();
    connections.pop();
  }
  else {
    ret.fd = -1;
  }
  q_mtx.unlock();
  return ret;
}



void CONN_HANDLERS::fill_queue(CONN_QUEUE* cq,int m_fd) {
  CONN_QUEUE::CONN connection;
  socklen_t addrlen = sizeof( *(connection.sockaddress) );
  while(true) {
    connection.fd = accept(m_fd, (sockaddr*)&connection.sockaddress, &addrlen);
    cq->push(connection);
  }
}

void CONN_HANDLERS::worker_thread(void( *operation )(int&,sockaddr_storage&),CONN_QUEUE* cq,bool auto_close = false) {
  CONN_QUEUE::CONN connection;
  while(true) {
    cq->wait();
    connection = cq->pop();
    if(connection.fd >= 0) {
      operation( connection.fd , *connection.sockaddress );
      if(auto_close) close(connection.fd);
    }
  }
}

Server.hpp

 // Server.hpp
#ifndef P_SERVER_H
#define P_SERVER_H
#include <thread>
#include <sys/socket.h>
#include <sys/types.h>
#include "Conn_Queue.hpp"

class Server {
  int m_fd;
  void (*hndlr) (int&,sockaddr_storage&);
  CONN_QUEUE cq;
  bool ac; // auto close
  public:
  Server(void (*h)(int&,sockaddr_storage&), const char* port, bool auto_close/* = false */);

  ~Server();

  std::vector<std::thread> start(int pool_size = 5);
};

#endif 

Server.cpp

 // Server.cpp
// un comment headers once you are using them.
#include "Conn_Queue.hpp"
#include "Server.hpp"
#include <thread>
#include <unistd.h> // close()
#include <sys/socket.h> // socket(), getaddrinfo(), freeaddrinfo(), bind()
#include <netdb.h> // addrinfo, 
#include <cstring> // memset(), 
//#include <sys/select.h> // hm...
#include <cstdio> // perror()
#include <cstdlib> // exit()

Server::Server( void(*h)(int&,sockaddr_storage&) , const char* port, bool auto_close = false) {
  // m_fd = socket(AF_UNSPEC, SOCK_STREAM, 0);
  hndlr = h;
  ac = auto_close;
  addrinfo hints, *servInfo;
  memset(&hints,0,sizeof(hints));
  // pack in some hints 
  hints.ai_flags = AI_PASSIVE;
  hints.ai_socktype = SOCK_STREAM;
  hints.ai_family = AF_UNSPEC;
  // get, well, addrinfo! lol.
  if(getaddrinfo(/* const char *__restrict __name */ NULL , port , &hints , &servInfo) != 0) {
    perror("call to getaddrinfo()");
    exit(1);
  }
  // make the socket.
  m_fd = socket(  servInfo->ai_family  ,  servInfo->ai_socktype  ,  servInfo->ai_protocol  );
  if(m_fd < 0) {
    perror("call to socket()");
    exit(1);
  }

  /*
  // make sure you can bind to your port by FORCING IT TO BE SO!! >:) ... just as soon as I figure out how this works.. XD
  if (setsockopt(listener,SOL_SOCKET,SO_REUSEADDR,&yes,sizeof yes) == -1) { 
    perror("an error occured during call to setsockopt()");
    exit(1);
  }
  */

  // bind socket to port
  if( bind(  m_fd  ,  servInfo->ai_addr  ,  servInfo->ai_addrlen  ) != 0 ){
    perror("call to bind()");
    exit(1);
  }

  /* just a quick note, NOT FOR SERVER, USE IN CLIENT!!!
  if( connect(  m_fd  ,  servInfo->ai_addr  ,  servInfo->ai_addrlen  ) < 0 ){
    perror("an error occured during call to connect()");
    exit(1);
  }
  */

}

Server::~Server() {
  close(m_fd);
}

std::vector<std::thread> Server::start(int pool_size/* = 5 */) {
  if(listen( m_fd , 5 ) < 0) {
    perror("call to listen()");
    exit(1);
  }
  std::vector<std::thread> ret;
  for(int i = 0;i < pool_size;i++)
    ret.push_back( std::thread(CONN_HANDLERS::worker_thread, hndlr, &cq, ac) );
  ret.push_back( std::thread(CONN_HANDLERS::fill_queue,&cq,m_fd) );
  return ret;
}

[highwayman]

Gah I forgot to say the most important thing:
When I get an address and store it in my CONN struct, the address is actually a list of several addresses, and I only use the first, so I was wondering if anyone knows a way to easily extract just that first address?

[salem c]

OK, some thoughts from me.

1. All upper case names like CONN_HANDLERS are typically reserved for pre-processor #defines.

2. Commenting a cryptic name with a long name is poor.
bool ac; // auto close
Just say
bool auto_close;

3. Create a naming convention for your classes, member functions and member variables.
https://duckduckgo.com/?q=c%2B%2B+naming+style

4. Typedef your function pointers.
typedef void (*handler_fn_t) (int&,sockaddr_storage&);

Then everywhere else becomes much clearer.

  handler_fn_t hndlr;
  CONN_QUEUE cq;
  bool ac; // auto close
  public:
  Server(handler_fn_t h, const char* port, bool auto_close/* = false */);

Or look at https://www.cplusplus.com/reference/functional/bind/ which gives you much more scope than a raw C-Style function pointer (for example being able to refer to class member functions and class instances).

[highwayman]

1. Sorry, kinda got a little carried away there, thinking maybe I’lll just make them UpperPascal instead like I should have.
2. Ok.. the reason ?I had it like that is because I was trying to make it more clear in the constructor why I have a bool. But yeah I’ll figure something out. Ok.
3. classes: UpperPascal , variables & functions: snake_case. hm. Maybe I’ll change variables to CamelCase, but I really like snake_case lol.
4. I’ve been mulling over that one for so long! Thank you! That’s perfect. Yeah I think I’ll use bind.

Thank you so much for the feedback :)

[highwayman]

Oh yeesh looking up naming conventions, they have a lot more than I was thinking about lol.

[deleted account xyzzy]

In addition to std::bind there is std::function.
http://www.cplusplus.com/reference/functional/function/function/

Consider, ummmmm, using using over typedef. The two are equivalent, but if you get into writing templates (alias templates specifically) typedef is apparently not allowed.
https://stackoverflow.com/questions/10747810/what-is-the-difference-between-typedef-and-using-in-c11

IMO a using alias for a function looks easier to understand what is happening.

I don't know what your OS and compiler is. If you were using Visual Studio (2017 or 2019) it allows for easy global renaming of variables. I use it a lot if I am mashing on code written by others.

[highwayman]

1. Maybe. I’ll probably waffle between the two for a minute or two 🤷‍♂️.
2. Oh, uh. I didn’t even realize that using could do that actually. Ok.
3. Ubuntu 18.04.01...? I think? And clang or gcc.

Edit: actually std::function is a lot clearer to me.

[deleted account xyzzy]

No matter what the editor global text searches and replacing shouldn't be a burden. I simply mentioned VS because it provides a nice popup window for scrolling through the code to see what gets changed at a global level.

[highwayman]

Ah ok. That does sounds nice actually :P

[deleted account xyzzy]

I didn’t even realize that using could do that actually.

I keep finding out new uses for language features all the time. Both by reading what more knowledgeable people post in code here, and by searching the inter-webs to help answer questions.

I remembered using is a "new C++ code" typedef replacement. But was a bit vague as to the extent.

My answer (and the link) was from a 'net search to confirm what I half-remembered.

[highwayman]

That’s basically my life :P

[deleted account xyzzy]

At least we two are willing to stretch our coding muscles and go outside of our comfort zone when a better way to do something is possible.

Most times I have to be dragged, kicking and screaming, into seeing something. But once the hysterics and stampy-foot tantrums are over I realize I should be grateful for the challenge.

Helps me to remember there's something I should remember to use in future code.

I can also be somewhat militantly evangelical about certain things that most beginners are taught, and IMO taught wrongly.

using namespace std; is a good example. Another is using the C library random generator when C++ has "a killer app for that."

[highwayman]

I’m not entirely sure I do do that :/ I’m much to attached to making my own version of things, generally because I just can’t wrap my head around the normal STL implementation or other lib someone has presented me with. It’s a bad habit of mine lol.

Hm. The list of things I think I’ve ever forced myself to do for coding is quite small, maybe two things, but I definitely see where your coming from. Those two thing did definitely help like that.

I’m never entirely sure what to think of things like that. For example: using namespace std; is a great wait to save namespaces and mangling until later and can clean up code for a beginner, but it’s a terrible habit to have.
That actually makes me think of when I include headers like <sys/socket.h> or <unistd.h>: they always throw a million errors if you don’t include them after your standard headers simply because they don’t have mangled names / namespaces. It’s so frustrating sometimes.