2

I'm working on a multithreaded UDP listener and I'm stuck in a problem that definitely surpasses me.

So, I'm required to receive huge amounts of UDP packets in several ports. Locally, the best solution for me was to call non blocking recvfrom in as much threads as ports I'm listening (select and poll were too slow for my requirements). I'm using a thread pool manager, it simply calls on threads and queues tasks. Here's the code:

void receiveFromSocket(void * arguments){

    sockaddr_in client;  // Local
    socklen_t clientSize = sizeof(client);
    memset(&client, 0, sizeof(client));
    struct arg_struct_listenPort *args2 = (struct arg_struct_listenPort *)arguments;
        int fd = args2->arg_fd;
        int port = args2->arg_port;

    for(;;) {
        char buf[158];
        memset(buf,0,158*sizeof(char));
        int n = recvfrom(fd, (char * ) buf, 158, MSG_DONTWAIT, ( struct sockaddr *) &client, &clientSize);

            if(n == -1){
               //cerr << "Error while receiving from client: " << errno << endl;
               continue;
            }

            if(n != 158){
               cerr << "Discarded message since it's not 158 bytes." << endl;
               continue;
            }
            struct arg_struct args;
                args.arg_port = port;
                memcpy(args.buf,buf,158);

            thpool_add_work(globals.thpool, socketThread, (void*)(&args));

    }

}


/// Runs the Socket listener
int network_accept_any()
{
        vector<int>::iterator i;
        for(i = globals.fds.begin(); i != globals.fds.end(); i++){
            int port = distance(globals.fds.begin(),i);
            struct arg_struct_listenPort args;
                args.arg_fd = *i;
                args.arg_port = globals.cmnSystemCatalogs[port].diag_port;
            thpool_add_work(globals.thpool, receiveFromSocket, (void*)(&args));
        }
        cout << "Listening threads created..." << endl;

    return 0;
}

This works perfectly fine locally. But when I compile it on a production environment, some ports listen the packets and other's simply don't! And the working ports change in each execution. I can , confirm that it is not a firewall problem. I also can clearly see the packets through Wireshark. I can receive packets on those ports through netcat. Netstat shows all ports open.

My local environment is an Ubuntu 18.04 VM, and the production environment is a Debian 9.8.

Here's how I call the sockets:

int lSocket(int port) {

    //Crear Socket
        int listening = socket(AF_INET, SOCK_DGRAM, 0);
        if (listening == -1) {
            cerr << "No se puede crear el socket";
            exit(EXIT_FAILURE);
        }

        //Enlazar socket a un IP / puerto

        struct sockaddr_in hint;
        memset(&hint, 0, sizeof(hint));
        hint.sin_family = AF_INET; //IPv4
        hint.sin_port = htons(port); //Port
        hint.sin_addr.s_addr = htonl(INADDR_ANY);

        if(bind(listening, (struct sockaddr*)&hint, sizeof(hint)) == -1) { //Enlaza las opciones definidas al socket
            cerr << "No se puede enlazar IP/puerto" << endl;
            exit(EXIT_FAILURE);
        }


        return listening;

}

Any advise is greatly appreciated!

EDIT:

As suggested, I tried switching to blocking I/O, but the main issue remains. Still not receiving at all the opened ports.

Jose M
  • 104
  • 1
  • 7
  • Language note: the units of data traversing an IP network are called "**packets**" not "packages". – John Bollinger Mar 12 '19 at 11:59
  • If you are devoting as many threads to the task as you have ports, then it seems at best useless to perform non-blocking I/O. At worst, it could contribute to your problem, as you will have a lot of busy-looping. Usually, it's a *choice* between multithreading and non-blocking I/O. Moreover, with one thread per port, a pool probably contributes nothing useful. Dedicating one thread to performing *blocking* I/O at each port is the way I would apply multithreading to such a task. – John Bollinger Mar 12 '19 at 12:02
  • Thanks for your comments, John. I understand what you mean. But I'm receiving packets at a great rate and using blocking I/O was making me receive the same packet several times. Anyhow, it should be able to receive at least one packet at each port, right? – Jose M Mar 12 '19 at 12:07
  • 4
    UDP, by definition is not guaranteed. No matter what you do, you have no guarantees, whatsoever, that a sent packet will be delivered. Furthermore if "using blocking I/O was making me receive the same packet several times", this indicates a bug somewhere in your code. There's nothing inherent in the default "blocking I/O" that ends up duplicating packets. You will simply need to debug your own code.' – Sam Varshavchik Mar 12 '19 at 12:17
  • 3
    You're using pointers to objects whose lifetime has ended (`&args`). This has undefined behaviour - it might appear to work, but it's a bug that needs a-fixin'. – molbdnilo Mar 12 '19 at 12:30
  • Welcome to SO! As moldnilo already said, first fix any UB, any reasoning before that is non viable. Then I guess you might trigger some OS DOS protection or something, however to be sure it not that, one has to try to reproduce it on many plattforms. Also try to minimize your code further, it always pays out. And don't spawn as many threads as ports, thats oversubscription, use `std::thread::hardware_concurrency` or a reasonable number between 1 and 4. – Superlokkus Mar 12 '19 at 12:40
  • @Superlokkus, running as many threads as ports does not necessarily constitute oversubscription. In particular, at any given time, processing resources are consumed only by those that are not blocked, so dozens of concurrent threads can be fine if most of them are (say) blocked on I/O. This is another reason to prefer blocking I/O in this scenario if data is arriving at more ports than the machine has cores with which to service them. – John Bollinger Mar 12 '19 at 12:45
  • @JohnBollinger not necessarily but possibly. But yeah a thread per port synchron model is ok, but IMHO just a compromise regarding scale and performance. However as we can't see the full implementation, busy waiting is possible. There might be a good reason the async proactor model had become more popular, as supported natively by most OSes and libraries, rather than the java like syhcron reactor pattern. – Superlokkus Mar 12 '19 at 14:49

1 Answers1

1

What an amazing welcome!

@molbdnilo was absolutely right:

You're using pointers to objects whose lifetime has ended (&args). This has undefined behaviour - it might appear to work, but it's a bug that needs a-fixin'.

Here's the fixed code. Gotta be careful when feeding arguments to threads!

void receiveFromSocket(void * arguments){

    sockaddr_in client;  // Local
    socklen_t clientSize = sizeof(client);
    memset(&client, 0, sizeof(client));
    struct arg_struct_listenPort *args2 = (struct arg_struct_listenPort *)arguments;
        int fd = args2->arg_fd;
        int port = args2->arg_port;

    for(;;) {
        char buf[158];
        memset(buf,0,158*sizeof(char));
        int n = recvfrom(fd, (char * ) buf, 158, MSG_WAITALL, ( struct sockaddr *) &client, &clientSize);

            if(n == -1){
               cerr << "Error while receiving from client: " << errno << endl;
               continue;
            }

            if(n != 158){
               cerr << "Discarded message since it's not 158 bytes." << endl;
               continue;
            }
            arg_struct *args = new arg_struct;
                args->arg_port = port;
                memcpy(args->buf,buf,158);

            thpool_add_work(globals.thpool, socketThread, (void*)(args));

    }

}


/// Runs the Socket listener
int network_accept_any()
{
        vector<int>::iterator i;
        for(i = globals.fds.begin(); i != globals.fds.end(); i++){
            int port = distance(globals.fds.begin(),i);
          arg_struct_listenPort *args = new arg_struct_listenPort;
                args->arg_fd = *i;
                args->arg_port = globals.cmnSystemCatalogs[port].diag_port;
            thpool_add_work(globals.thpool, receiveFromSocket, (void*)(args));
        }
        cout << "Listening threads created..." << endl;


    return 0;
}

Also, I'll keep an eye on @John Bollinger 's and @Superlokkus comments.

Thank you all!

Jose M
  • 104
  • 1
  • 7