4
\$\begingroup\$

The other day I ran into this question on stackoverflow. Just for curiosity I wanted to implement suspending and resuming the thread, so I used signals and used pause() in the signal handler.

Is it OK to use this way? This question has many answers how to suspend/resume a thread, but I want to know what is wrong with my approach.

As far as I know signal handlers should be treated as interrupts i.e. minimum code and immediate return from it.

According to this, pause() is async-signal safe system call.

#include <stdio.h>
#include <unistd.h>
#include <pthread.h>
#include <signal.h>

// Since I have only 2 threads so using two variables, 
// array of bools will be more useful for `n` number of threads.
static int is_th1_ready = 0;
static int is_th2_ready = 0;

static void cb_sig(int signal)
{
        switch(signal) {
        case SIGUSR1:
                pause();
                break;
        case SIGUSR2:
                break;
        }
}

static void *thread_job(void *t_id)
{
        int i = 0;
        struct sigaction act;

        pthread_detach(pthread_self());
        sigemptyset(&act.sa_mask);
        act.sa_flags = 0;
        act.sa_handler = cb_sig;

        if (sigaction(SIGUSR1, &act, NULL) == -1)
                printf("unable to handle siguser1\n");
        if (sigaction(SIGUSR2, &act, NULL) == -1)
                printf("unable to handle siguser2\n");

        if (t_id == (void *)1)
            is_th1_ready = 1;
        if (t_id == (void *)2)
            is_th2_ready = 1;

        while (1) {
                printf("thread id: %p, counter: %d\n", t_id, i++);
                sleep(1);
        }

        return NULL;
}

int main()
{
        int terminate = 0;
        int user_input;
        pthread_t thread1, thread2;

        pthread_create(&thread1, NULL, thread_job, (void *)1);
        // Spawned thread2 just to make sure it isn't suspended/paused 
        // when thread1 received SIGUSR1/SIGUSR2 signal
        pthread_create(&thread2, NULL, thread_job, (void *)2);

        while (!is_th1_ready && !is_th2_ready);

        while (!terminate) {
                // to test, I am sensing signals depending on input from STDIN
                printf("0: pause thread1, 1: resume thread1, -1: exit\n");
                scanf("%d", &user_input);

                switch(user_input) {
                case -1:
                        printf("terminating\n");
                        terminate = 1;
                        break;
                case 0:
                        printf("raising SIGUSR1 to thread1\n");
                        pthread_kill(thread1, SIGUSR1);
                        break;
                case 1:
                        printf("raising SIGUSR2 to thread1\n");
                        pthread_kill(thread1, SIGUSR2);
                        break;
                }
        }

        pthread_kill(thread1, SIGKILL);
        pthread_kill(thread2, SIGKILL);

        return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Jun 17, 2019 at 10:10

1 Answer 1

3
\$\begingroup\$

This is a useful question because it illustrates how a seemingly working program using pthreads can be loaded with pitfalls. I hope that you take this answer in the spirit in which it is intended, which is to help you improve your programming. I didn't see any major problems with the way you're using signals.

Consider using standard threads

You may already know this, but since C11, threads are now part of the standard. Using them instead of POSIX threads (pthreads) could make the program, in theory anyway, somewhat more portable. I'll make no further mention of that, however, and the rest will deal with pthreads, although most of the suggestions still apply.

Think carefully about using non-standard calls

The sigemptyset() call is neither a C nor a POSIX standard. While it's widely supported, it isn't really portable, strictly speaking. That may be fine for your purposes, but it's important to know which calls are standard and which are not to improve the portability of your program. (I was actually thinking of sigisemptyset here — my mistake!)

Use a mutex to access shared resources

Both threads are attempting to use printf but that's a problem because that requires access to a shared resource, namely stdout. Access printf within POSIX threads are guaranteed to be thread safe, but the output could be scrambled (that is, the letters from two different threads could be intermixed). One could use a mutex to assure that only one thread at a time is using that resource. Here's one way to do that:

#include <stdarg.h>

static pthread_mutex_t stdout_mutex = PTHREAD_MUTEX_INITIALIZER;

static int locking_printf(const char *format, ...) {
    va_list args;
    va_start(args, format);
    pthread_mutex_lock(&stdout_mutex);
    int retval = vprintf(format, args);
    pthread_mutex_unlock(&stdout_mutex);
    va_end(args);
    return retval;
}

Then replace every instance of printf with locking_printf, including the ones in main since that's a thread, too.

Match return types with function

There's no need to write return NULL; at the end of thread_job because it's a void function. Also, it's my view that one should omit return 0; from the end of main. It's always contentious because others disagree (often very passionately!), but here's the rationale.

Use bool types where appropriate

The is_th1_ready and is_th2_ready variables are more appropriately described as bool rather than int variables. For that reason, I'd recommend using <stdbool.h> and writing their declaration and initialization like this:

static bool is_th1_ready = false;
static bool is_th2_ready = false;

Think about casting

Creating the threads uses this kind of call:

pthread_create(&thread2, NULL, thread_job, (void *)2);

That's fine and appropriate. However, the later use of that data within the thread is a bit unusual:

if (t_id == (void *)1)
    is_th1_ready = 1;

It's perfectly technically valid, but the more conventional approach is to cast the passed void * into something intelligible to the local thread -- often this is a pointer to a struct containing multiple values. In this case, I'd probably write it like this instead:

if ((int)t_id == 1)
    is_th1_ready = true;

On my machine this causes a compiler warning because int and void * are not the same size (on this machine, with this compiler), but that's not a problem because we know with certainty that this is, in fact, an int. Another approach would be to do the cast once in the creation of a local int variable.

Rethink the use of scanf

If the user enters a letter or some other non-numeric input, the scanf input fails and the user loses control of the application. It's well known that scanf has problems. For that reason, I'd recommend instead using fgets and then strtod.

char buf[5];  // we don't need a big buffer
fgets(buf, sizeof buf, stdin);
switch(strtol(buf, NULL, 10)) {

This will interpret any "garbage" input as 0, but that's probably OK for this program. Note also that for POSIX systems, non-numeric input can be detected by looking at the errno value.

Prefer for to while where appropriate

The only use of terminate is within the loop, so I'd suggest changing from while to for like this:

for (bool running = true; running; ) {

Note that I've inverted the sense to eliminate the need to negate. The compiler would probably have done something like that anyway, but I think it's more clear for human readers this way.

Don't bother killing threads at exit

The return from main or a call to exit() will both cause the threads to be cleaned up and resources returned, so I would probably omit the two pthread_kill calls at the end of main.

Consider more robust error handling

It's nice that the code is checking the return value of sigaction, but should the thread even launch if we can't handle the signals? It's a design choice, but one worth considering, in my opinion.

\$\endgroup\$
6
  • \$\begingroup\$ One possible reason to clean up when exiting is if using Valgrind or similar tool, and don't want these to show up as unreleased resources. In which case, it should be an optional behaviour (not the default). \$\endgroup\$ Jun 17, 2019 at 17:34
  • \$\begingroup\$ @TobySpeight: True. Another approach is to not detach the threads and to use join instead. If I were writing this, that's how I'd do it because in this case, there is no reason that one would want the threads truly running detached (e.g. running independently after main ends). \$\endgroup\$
    – Edward
    Jun 17, 2019 at 17:49
  • \$\begingroup\$ Quite right - that was a general comment on motivation, without actually studying the code! I was extrapolating from "unnecessary" memory release. Yes, joining is the most appropriate way to clean up worker threads. \$\endgroup\$ Jun 17, 2019 at 18:10
  • \$\begingroup\$ I Understood the point of not using standard functions for the ease of portability. sigemptyset is confirming to POSIX.1-2001, POSIX.1-2008 standards. -- from the man page. \$\endgroup\$
    – Shubham
    Jun 18, 2019 at 3:44
  • 1
    \$\begingroup\$ The use of pause looks fine to me, and you’re right about sigemptyset - it’s sigisemptyset (which your code doesn’t use) that’s not POSIX. I’ll correct my answer. \$\endgroup\$
    – Edward
    Jun 18, 2019 at 4:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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