4
\$\begingroup\$

Referring to this post, my problem (summarized):

One main thread waits for connections, on accept() spawns a new thread for each connection. After we received n connections, the main thread dictates all threads what command to run, and must wait for all threads to finish, until issuing a new command later on.

I would love to know if this solves my problem efficiently and is correct.

static int mCommandToExecute = 0;
static int mNumberConnectedClients = 0;
static pthread_barrier_t mWaitForAllDone;
static pthread_barrier_t mWaitForCommand;

void* handleRequest(void* arg) {

    ...

    while (1) {
        // Wait for a command here
        pthread_barrier_wait(&mWaitForCommand);    

        printf("\nSending to client: %s\n", commands[mCommandToExecute]);
        n = write(sockfd, commands[mCommandToExecute], length[mCommandToExecute]);
        ...

        // Wait for all threads to finish here 
        pthread_barrier_wait(&mWaitForAllDone);
    }
}

void* startServer(void* arg) {
    pthread_barrier_init(&mWaitForCommand, NULL, maxConnections + 1);
    pthread_barrier_init(&mWaitForAllDone, NULL, maxConnections + 1);

    for (mNumberConnectedClients = 0; mNumberConnectedClients < maxConnections; ++mNumberConnectedClients) {
        clientSocket = accept(sockfd, (struct sockaddr *) &client_addr, &clientLength);
        pthread_create(&mThread_pool[mNumberConnectedClients], NULL, handleRequest, (void*) &clientSocket);
    }

    // Wait for all threads to have booted up and ready to execute a command
    mCommandToExecute = 0;
    for (int i = 0; i < 4; ++i) { // Run through 4 commands as an example
        pthread_barrier_wait(&mWaitForCommand);
        // At least one thread is still executing here
        pthread_barrier_wait(&mWaitForAllDone);
        // All commands are done here
        mCommandToExecute = i + 1; // No mutex is used here, so set variable before every threads reads it
    }

    // Wait for all threads to finish
    for (int i = 0; i < maxConnections; ++i) {
        pthread_join(mThread_pool[i], NULL);
    }
}
\$\endgroup\$
3
\$\begingroup\$

The threading mechanics mostly look correct. Your main thread and your subthreads only share the mCommandToExecute and commands variables. The subthreads only read them between mWaitForCommand and mWaitForAllDone, and the main thread reads and writes the mCommandToExecute between mWaitForAllDone and waitForCommand. So no one is writing to this data while anyone else is reading it, and your data is thread-safe.

However your subthreads do not have any exit condition, so I expect the pthread_join will hang on the first one. When you reach that point, all your subthreads will be waiting for the next command. Joining the thread does not terminate it. It simply allows you to synchronize their termination. You need to add some signal for the subthreads to terminate, and then you need to invoke that signal and then pthread_barrier_wait(mWaitForCommand) to allow each subthread to proceed to check the termination signal.

One simple way to do this, if it's practical, would be to make the termination step the final "command". Rather than sending the termination command, the subthreads could recognize it and just return immediately. Your main thread would need to avoid waiting on mWaitForAllDone for that last command, though.

I assume you will have some other code which handles lost connections, closing the accepted connections after the threads terminate, etc. You do not seem to allow for stalled threads such as might occur in many network situations. If a client does stall, it will stall your entire system of threads. I assume this is intentional or else you have timeouts and handlers for such occurrences in your real code.

\$\endgroup\$
  • \$\begingroup\$ Hi, and welcome to Code Review - this post has successfully made it through the First Post review queue. \$\endgroup\$ – rolfl Dec 15 '14 at 3:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.