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:
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
Use
volatile sig_atomic_t
for shared variable updates: You already usedvolatile sig_atomic_t
for the flag variable e_flag, which is good for atomic updates. However, you also usedsize_t
for iterating the threads, which is not guaranteed to be atomic. It's better to use volatilesig_atomic_t
in this case as well.// The first half is already done, what does it mean in the 2nd half?
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
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
Don't catch all errors in
pthread_create()
:pthread_create()
may fail for reasons other thanENOMEM
. You should check the return value and exit or continue the loop accordingly.//
man pthread_create
doesn't listENOMEM
as an error. Possible errors areEAGAIN
,EINVAL
andEPERM
only. But the point it raises does appears to be worth exploring.Use
size_t
for thread indexes: Sincepthread_create()
expects avoid*
parameter, you can pass any type of pointer to it. It's better to usesize_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.
signal_handler
, did you mean to write toSTDOUT_FILENO
orSTDERR_FILENO
? \$\endgroup\$-pedantic
is a superset of this but I always throw in-Wextra
. \$\endgroup\$-Wextra
added \$\endgroup\$STDOUT_FILENO
, odd it still works even if I write toSTDIN_FILENO
.. \$\endgroup\$