Use a dedicated thread to process signals #105

2020-02-19T01:53:30+01:00

sth commented

2020-02-19 01:53:30 +01:00

(Migrated from github.com)

I put the signal handling in a dedicated thread which should be able to use malloc/logging/... without causing problems. (See #103)

Handling SIGSGEV/SIGILL/SIGBUS is implemented but likely pointless: If a thread runs into an instruction that triggers one of those it will be impossible for the thread to continue even though the signal is blocked for the thread. The kernel will the quickly kill the program despite the blocked signal, most likely before the signal thread gets a chance to run.

I made close_signals static to make sure it doesn't go out of scope while the signal thread might still be running.

I put the signal handling in a dedicated thread which should be able to use malloc/logging/... without causing problems. (See #103) Handling SIGSGEV/SIGILL/SIGBUS is implemented but likely pointless: If a thread runs into an instruction that triggers one of those it will be impossible for the thread to continue even though the signal is blocked for the thread. The kernel will the quickly kill the program despite the blocked signal, most likely before the signal thread gets a chance to run. I made `close_signals` static to make sure it doesn't go out of scope while the signal thread might still be running.

babelouest (Migrated from github.com) reviewed

2020-02-19 05:16:29 +01:00

src/glewlwyd.c

I prefer either to declare functions in glewlwyd.h or implement them above the calling function. In this case I think declaring signal_thread in glewlwyd.h will be sufficient and in harmony with the code style.

I prefer either to declare functions in `glewlwyd.h` or implement them above the calling function. In this case I think declaring `signal_thread` in `glewlwyd.h` will be sufficient and in harmony with the code style.

src/glewlwyd.c

					
				@ -56,2 +56,4 @@

				  pthread_t signal_thread_id;

				  static sigset_t close_signals;

				  if (config == NULL) {

Why is this variable static? Is it to be invisible in the other files?

src/glewlwyd.c

					
				@ -181,15 +183,27 @@ int main (int argc, char ** argv) {

				      pthread_cond_init(&global_handler_close_cond, NULL)) {

				    y_log_message(Y_LOG_LEVEL_ERROR, "init - Error initializing global_handler_close_lock or global_handler_close_cond");

				  }

I think we should test the result of sigemptyset and sigaddset to be 0, just to be sure

I think we should test the result of `sigemptyset` and `sigaddset` to be 0, just to be sure

src/glewlwyd.c

I'm not sure putting an infinite loop here is relevant. If I understand this function correctly, it waits until an expected signal is sent, then broadcasts a closing signal with global_handler_close_cond or exits if the signal is bad.
In any case, when the signal is received, it will close the program, or there may be other signum values?

If we need to keep the while (1) {, then I'd add a usleep(50) right before the end loop to avoid having the CPU at 100%.

I'm not sure putting an infinite loop here is relevant. If I understand this function correctly, it waits until an expected signal is sent, then broadcasts a closing signal with `global_handler_close_cond` or exits if the signal is bad. In any case, when the signal is received, it will close the program, or there may be other `signum` values? If we need to keep the `while (1) {`, then I'd add a `usleep(50)` right before the end loop to avoid having the CPU at 100%.

Here I'd replace the y_log_message(Y_LOG_LEVEL_ERROR with fprintf(stderr because y_log_message's malloc may cause other problems during a program failure.

Here I'd replace the `y_log_message(Y_LOG_LEVEL_ERROR` with `fprintf(stderr` because `y_log_message`'s `malloc` may cause other problems during a program failure.

babelouest commented

2020-02-19 05:17:51 +01:00

(Migrated from github.com)

Wow, thanks @sth for the help! I really appreciate the initiative.

I made some comments, nothing big but some questions and ways to make the code cleaner.

Wow, thanks @sth for the help! I really appreciate the initiative. I made some comments, nothing big but some questions and ways to make the code cleaner.

sth (Migrated from github.com) reviewed

2020-02-19 11:32:26 +01:00

src/glewlwyd.c

Practically there won't be other signum values, because we only wait for signals in sigs and we check for all of them manually. All signals we receive will end up exiting the thread (and program) in some way, so the infinite loop is not really needed.

It would be useful if there were other signals that would be handled without exiting (typical would be "reload configuration on SIGHUP" or something like that). Then we really should continue to wait for the more signal that might come. I'll add a log warning for any unexpected received signals, even though it should never happen currently. It's a bit more resilient in case more signals are added later. But the loop could also be removed if you prefer it.

I don't think a sleep is really necessary: if there are signals generated at a rate that takes 100% CPU to process, than the real problem is wherever those signals are generated. The thread processing them can't really be blamed that many of them are coming in.

Practically there won't be other signum values, because we only wait for signals in `sigs` and we check for all of them manually. All signals we receive will end up exiting the thread (and program) in some way, so the infinite loop is not really needed. It would be useful if there were other signals that would be handled without exiting (typical would be "reload configuration on SIGHUP" or something like that). Then we really should continue to wait for the more signal that might come. I'll add a log warning for any unexpected received signals, even though it should never happen currently. It's a bit more resilient in case more signals are added later. But the loop could also be removed if you prefer it. I don't think a sleep is really necessary: if there are signals generated at a rate that takes 100% CPU to process, than the real problem is wherever those signals are generated. The thread processing them can't really be blamed that many of them are coming in.

sth (Migrated from github.com) reviewed

2020-02-19 11:45:12 +01:00

src/glewlwyd.c

					
				@ -56,2 +56,4 @@

				  pthread_t signal_thread_id;

				  static sigset_t close_signals;

				  if (config == NULL) {

I made it static because a pointer to it is passed to the signal thread and I wasn't sure if that thread might still be alive (for a short time) after the local variables in main() go out of scope.

We currently don't explicitly stop that thread before exiting main. Doing so would probably be more correct, but actually won't help anyway because the program exits by calling exit(), not by returning from main. I'm not sure if in this case it's guaranteed that other threads are cleaned up before local variables in main() might be overwritten. If the variable is static it definitely is valid until program exit.

I made it static because a pointer to it is passed to the signal thread and I wasn't sure if that thread might still be alive (for a short time) after the local variables in main() go out of scope. We currently don't explicitly stop that thread before exiting main. Doing so would probably be more correct, but actually won't help anyway because the program exits by calling `exit()`, not by returning from main. I'm not sure if in this case it's guaranteed that other threads are cleaned up before local variables in `main()` might be overwritten. If the variable is `static` it definitely is valid until program exit.

sth (Migrated from github.com) reviewed

2020-02-19 11:46:37 +01:00

src/glewlwyd.c

Yeah, makes sense to keep it simple in these cases.

sth (Migrated from github.com) reviewed

2020-02-19 11:53:25 +01:00

src/glewlwyd.c

					
				@ -181,15 +183,27 @@ int main (int argc, char ** argv) {

				      pthread_cond_init(&global_handler_close_cond, NULL)) {

				    y_log_message(Y_LOG_LEVEL_ERROR, "init - Error initializing global_handler_close_lock or global_handler_close_cond");

				  }

Definitely!

babelouest (Migrated from github.com) reviewed

2020-02-19 13:17:24 +01:00

src/glewlwyd.c

Then I would remove the while loop

babelouest (Migrated from github.com) reviewed

2020-02-19 13:21:13 +01:00

src/glewlwyd.c

					
				@ -1518,22 +1532,35 @@ void print_help(FILE * output) {

				 * handles signal catch to exit properly when ^C is used for example

Some indentation to improve here

babelouest (Migrated from github.com) reviewed

2020-02-19 13:22:26 +01:00

src/glewlwyd.c

					
				@ -1518,22 +1532,35 @@ void print_help(FILE * output) {

				 * handles signal catch to exit properly when ^C is used for example

And same as below, use fprintf before an emergency exit.

And same as below, use `fprintf` before an emergency exit.

babelouest (Migrated from github.com) reviewed

2020-02-19 13:46:14 +01:00

src/glewlwyd.c

Also, I think we should detach the thread to release its resource when it finishes.

if (pthread_create(&signal_thread_id, NULL, &signal_thread, &close_signals) || pthread_detach(&signal_thread_id)) {

Also, I think we should detach the thread to release its resource when it finishes. ```C if (pthread_create(&signal_thread_id, NULL, &signal_thread, &close_signals) || pthread_detach(&signal_thread_id)) { ```