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.

• Daniel Walker: In signal_handler, did you mean to write to STDOUT_FILENO or STDERR_FILENO?
• Daniel Walker: I don't know if -pedantic is a superset of this but I always throw in -Wextra.
• D.J. Elkind: @DanielWalker, -Wextra added
• Daniel Walker: That’s up to you but stdin is for input only.
• D.J. Elkind: @DanielWalker oh yes, it should be STDOUT_FILENO, odd it still works even if I write to STDIN_FILENO..

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.

• D.J. Elkind: All the suggestions until Do not cast the result of malloc(3) and family: are added to my revised version here. On the malloc() issue, if I dont add a cast, gcc complains: "warning: request for implicit conversion from ‘void *’ to ‘size_t *’ {aka ‘long unsigned int *’} not permitted in C++ [-Wc++-compat]". As I use both C and C++ , I tend to turn on -Wc++-compat so that I "use the subset of C that is valid in C++".
• D.J. Elkind: I made two relatively big changes to the signal_handler() part here. 1st: I make the C-string dynamic; 2nd: the write() has its error check, based on the comments from you and ChatGPT.
• D.J. Elkind: On "msg's length can be determined at compile-time with sizeof": my original thought is that my strlen() will be optimized away and replaced by a constant (I didn't check the disassembly though)
• D.J. Elkind: #include<string.h> is needed by strerror()
• Peter Cordes: @D.J.Elkind: Yes, it's true that stack space tends to be hot in cache, and that part of .data probably won't if you don't get signals often. But signals are usually so rare that it's probably best to minimize overall code+data footprint. Or if they're not, it will tend to still be hot in L2 or maybe L3 cache, and fewer instructions will execute every handler. Also here, where your access to it is write-only before the system call, the store buffer can hide the latency of a cache miss store, overlapping with the time the CPU is switching to kernel mode and eventually doing copy_from_user

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.

• D.J. Elkind: what is the benefit of doing so, compared with callpthread_join() in the main thread and use signal_handler() in the main thread?
• psmears: @D.J.Elkind: The biggest advantage is that, since you're not using a signal handler to do any work, you're not restricted in which functions you can call in response to the signal; it's also much easier to write code that doesn't have race conditions (which are a common issue with signal handlers due to their asynchronous nature).
• D.J. Elkind: @psmears I am actually not very sure how I can translate the words into code, do you have, say any links, that have some sample code which demonstrates the idea?
• psmears: @D.J.Elkind: I don't, but it shouldn't be that complicated: as the answer says, (1) in your main thread, before creating any other threads, call pthread_sigmask to block all signals (this stops the signals getting delivered to the wrong place); (2) have a thread (it can be the main thread or one created with pthread_create()) call sigwaitinfo() in a while loop to detect signals and process them.
• zwol: Yes, the biggest advantage of this approach is you never have to deal with signal concurrency (signals cause worse concurrency headaches than threads since they can interrupt critical sections, including critical sections inside the C library that you have no control over). Also, if there are multiple threads with a signal unblocked, then the kernel will choose one of them at random to run the signal handler, making things even more confusing than they already are.