Proper implementation of signal handler and multithreading (pthread)

Question

I am trying to implement a "proper" signal handler that works correctly for multiple threads, and to be as C-standard-conforming and POSIX-conforming as possible. The below is my implementation, any thoughts or bugs?:

#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <pthread.h>
#include <errno.h>

volatile sig_atomic_t e_flag = 0;

static void signal_handler(int signo) {
    char msg[] = "Signal caught\n";
    write(STDIN_FILENO, msg, strlen(msg));
    e_flag = 1;
}

void* event_loop(void* param) {
    int tid = *((int*)param);
    size_t iter_count = 0;
    while (e_flag == 0) {
        ++ iter_count;
        printf("Th %d: Event loop is running now, iterated %lu times ...\n",
            tid, iter_count);
        for (size_t i = 0; i < tid + 1 && e_flag == 0; ++ i) {
            sleep(1);
        }
    }
    size_t* ret = (size_t*)malloc(sizeof(size_t));
    if (ret != NULL) {
        *ret = iter_count;
    } else {
        perror("malloc()");
    }
    return ret;
}

int main() {
    if (signal(SIGINT, signal_handler) == SIG_ERR) {
        perror("signal()");
        return EXIT_FAILURE;
    }
    printf("A signal handler is installed, "
        "press Ctrl+C to exit event loop threads gracefully.\n");

    pthread_t threads[8];
    size_t running_thread_count = 0;
    /* It is not guaranteed that pthread_t is an int type, so we'd better
    create our own thread_id*/
    int tids[sizeof(threads) / sizeof(threads[0])];
    for (size_t i = 0; i < sizeof(threads) / sizeof(threads[0]); ++i) {
        tids[i] = i;
        int err_no;
        // pthread_create() doesn't set errno,
        // we need to catch its retval manually
        if ((err_no = pthread_create(
            &threads[i], NULL, event_loop, (void *)&tids[i])) != 0) {
            fprintf(stderr, "pthread_create() failed: %d(%s), "
                "the program exits now", err_no, strerror(err_no));
            if ((err_no = raise(SIGINT)) != 0) {
                fprintf(stderr, "even raise() failed: %d(%s), "
                    "the program exits UNgracefully",
                    err_no, strerror(err_no));
                exit(EXIT_FAILURE);
            }
            break;
        }
        ++running_thread_count;
    }

    for (size_t i = 0; i < running_thread_count; ++i) {
        size_t* ret;
        int err_no;
        // pthread_join() doesn't set errno,
        // we need to catch its retval manually
        if ((err_no = pthread_join(threads[i], (void**)&ret)) != 0) {
            fprintf(stderr, "pthread_join() failed: %d(%s), "
                "but there is nothing much we can do",
                err_no, strerror(err_no));
        } else {
            if (ret != NULL) {
                printf("Th %lu exited, iterated: %lu times\n", i, *ret);
                free(ret);
            } else {
                printf("Th %lu exited, but retval is not set as expected\n", i);
            }
        }
    }
    return EXIT_SUCCESS;
}

Corresponding Makefile in case you would like to compile:

OPTS = -O2 -Wall -pedantic -Wc++-compat
CC = gcc

main:   
    $(CC) main.c -o main.out $(OPTS) -lrt -lpthread

I also included ChatGPT 3.5's reply here, just for fun (I am aware that Stack Exchange bans using ChatGPT in answers, but I believe it doesn't ban it in questions?):

Overall, your implementation looks good and follows the best practices for handling signals in multi-threaded programs.

However, there are a few things you may want to consider:

1. Don't use printf() in a signal handler: According to the POSIX standard, the signal handler should avoid calling any function that is not async-signal-safe. printf() is not one of them, so it's recommended to use write() instead.

   // Well, I didn't do this

2. Use volatile sig_atomic_t for shared variable updates: You already used volatile sig_atomic_t for the flag variable e_flag, which is good for atomic updates. However, you also used size_t for iterating the threads, which is not guaranteed to be atomic. It's better to use volatile sig_atomic_t in this case as well.

   // The first half is already done, what does it mean in the 2nd half?

3. Check the return value of write(): write() may not write all the characters, especially if it's interrupted by a signal. You should check the return value to make sure all characters are written. This applies to any function that may be interrupted by a signal.

   If write() returns a value less than the size of the buffer, it means that not all characters were written. You should write the remaining characters in a loop until all of them are written. Here's an example of how to do this:

   static void signal_handler(int signo) {
       const char msg[] = "Signal caught\n";
       size_t len = sizeof(msg) - 1; // exclude null terminator
       size_t written = 0;
       while (written < len) {
           ssize_t ret = write(STDOUT_FILENO, msg + written, len - written);
           if (ret == -1 && errno == EINTR) {
               continue;
           }
           if (ret == -1) {
               perror("write()");
               break;
           }
           written += ret;
       }
       e_flag = 1;
   }
   

   // This seems a valid point

4. Don't exit in the signal handler: Exiting in the signal handler is not recommended, as it may cause undefined behavior. Also, raise() is not guaranteed to work in a signal handler. It's better to set the flag variable and let the main program exit gracefully.

   // Well, I didn't do this

5. Don't catch all errors in pthread_create(): pthread_create() may fail for reasons other than ENOMEM. You should check the return value and exit or continue the loop accordingly.

   // man pthread_create doesn't list ENOMEM as an error. Possible errors are EAGAIN, EINVAL and EPERM only. But the point it raises does appears to be worth exploring.

6. Use size_t for thread indexes: Since pthread_create() expects a void* parameter, you can pass any type of pointer to it. It's better to use size_t for the thread index, which is the same type as the array index. This avoids type casting and potential errors.

   // A valid point, fixed.

Answer 1

Use the correct prototype for main():

The C standard only defines two prototypes for main():

1) int main (void)
2) int main (int argc, char *argv[])
    Or 
   int main (int argc, char **argv)

The rest may be provided as an extension and are not portable.

signal(2) is deprecated and unportable:

From the Linux man page:

WARNING: the behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux.

The only portable use of signal() is to change the signal's disposition to SIG_IGN or SIG_DFL. Avoid it's use; consider sigaction(2) instead.

exit(3) performs normal program termination:

fprintf(stderr, "even raise() failed: %d(%s), "
                 "the program exits UNgracefully",
                  err_no, strerror(err_no));
exit(EXIT_FAILURE);

Perhaps you're looking for abort(3)?

The correct format specifier for size_t is %zu:

From the C standard:

If any argument is not the correct type for the corresponding conversion specification, the behavior is undefined.

//printf("Th %lu exited, but retval is not set as expected\n", i);
 
printf("Th %zu exited, but retval is not set as expected\n", i);

Do not cast the result of malloc(3) and family:

//size_t* ret = (size_t*)malloc(sizeof(size_t));
size_t* ret = malloc(sizeof(size_t));

malloc() returns a generic void * that is implicitly converted to any other pointer type. As such, it's result need not be casted and only serves to clutter one's code.

Minor:

char msg[] = "Signal caught\n";
write(STDIN_FILENO, msg, strlen(msg));

msg's length can be determined at compile-time with sizeof:

/* This seems to be a bug. Are we 
 * writing to STDIN_FILENO? — Peter Cordes
 */ 
write (STDIN_FILENO, msg, sizeof msg - 1);

Allocate to the referenced type:

size_t* ret = malloc(sizeof *ret);

Use more const:

//char msg[] = "Signal caught\n";
const char msg[] = "Signal caught\n";

param is not changed anywhere in event_loop, it deserves to be const-qualified:

void* event_loop(const void* param) {
   const  int tid = *param;

I do not have a compiler at hand, but there's a type mismatch here:

for (size_t i = 0; i < tid + 1 && e_flag == 0; ++ i) {

i is an unsigned integer type, whereas tid is a signed integer type. Enable more compiler warnings.

The return value of write(2) is left unchecked.

The inclusion of string.h is not required.

As for compliancy to the ISO C standard, it doesn't know anything about write().

C11 §7.14.1.1 The signal function ¶5 specifies the rules:

If the signal occurs other than as the result of calling the abort or raise function, the behavior is undefined if the signal handler refers to any object with static or thread storage duration that is not a lock-free atomic object other than by assigning a value to an object declared as volatile sig_atomic_t, or the signal handler calls any function in the standard library other than the abort function, the _Exit function, the quick_exit function, or the signal function with the first argument equal to the signal number corresponding to the signal that caused the invocation of the handler. Furthermore, if such a call to the signal function results in a SIG_ERR return, the value of errno is indeterminate.

Answer 2

In a multithreaded program, best practice is to devote a thread to signal handling.

Block all asynchronous signals¹ at the beginning of main, using pthread_sigmask, and leave them that way for the entire execution of the program (unless it spawns child processes; these will need to revert to the original signal mask before calling execve; but you do that only on the child side of the fork).

For each signal that you care about,² install a signal handler that does nothing; the only purpose of this is to ensure that none of these signals are set to be ignored.

Finally, create a thread that loops calling sigwaitinfo for all the signals you care about, and doesn't do any other work. Do all of the processing of signal events in this thread's loop, not in handlers. Communicate with other threads, using more ergonomic IPC mechanisms, as necessary. Keep in mind that this thread is effectively an event-driven I/O handler, regardless of the architecture of the rest of your program; strive to make it do as little work as possible, and return to sigwaitinfo as quickly as possible.

¹ That is, all signal numbers except: SIGABRT, SIGBUS, SIGEMT (might not be defined), SIGFPE, SIGILL, SIGIOT (might not be defined and is likely to be equal to SIGABRT if it is), SIGSEGV, SIGSYS, and SIGTRAP.

² This must include at least the signals that notify you of user-initiated cancellation or suspension of your program: SIGHUP, SIGINT, SIGPWR, SIGQUIT, SIGTSTP, SIGTERM, SIGTTIN, SIGTTOU, and SIGXCPU.