9

So I'm trying to call an alarm to display a message "still working.." every second. I included signal.h.

Outside of my main I have my function: (I never declare/define s for int s)

void display_message(int s);   //Function for alarm set up
void display_message(int s) {
     printf("copyit: Still working...\n" );
     alarm(1);    //for every second
     signal(SIGALRM, display_message);
}

Then, in my main

while(1)
{
    signal(SIGALRM, display_message);
    alarm(1);     //Alarm signal every second.

That's in there as soon as the loop begins. But the program never outputs the 'still working...' message. What am I doing incorrectly? Thank you, ver much appreciated.

vivav
  • 105
  • 1
  • 1
  • 5
  • possible duplicate of http://stackoverflow.com/questions/1784136/simple-signals-c-programming-and-alarm-function – txtechhelp Feb 04 '14 at 02:51

5 Answers5

18

Signal handlers are not supposed to contain "business logic" or make library calls such as printf. See C11 §7.1.4/4 and its footnote:

Thus, a signal handler cannot, in general, call standard library functions.

All the signal handler should do is set a flag to be acted upon by non-interrupt code, and unblock a waiting system call. This program runs correctly and does not risk crashing, even if some I/O or other functionality were added:

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

volatile sig_atomic_t print_flag = false;

void handle_alarm( int sig ) {
    print_flag = true;
}

int main() {
    signal( SIGALRM, handle_alarm ); // Install handler first,
    alarm( 1 ); // before scheduling it to be called.
    for (;;) {
        sleep( 5 ); // Pretend to do something. Could also be read() or select().
        if ( print_flag ) {
            printf( "Hello\n" );
            print_flag = false;
            alarm( 1 ); // Reschedule.
        }
    }
}
Potatoswatter
  • 126,977
  • 21
  • 238
  • 404
  • You can typically call non-signal-safe functions if you can _guarantee_ they can't be normally called by your process (directly or indirectly). `signal` and `alarm` are both signal safe, so as long as the program does not otherwise call any signal-unsafe functions in constructors/destructors, shared libraries, or other threads, the program in question is actually perfectly safe. "signal-unsafe" indicates a function *can* be unsafe, not that it *is*, and the "you can only set a `sig_atomic_t` flag" hasn't been true since POSIX. – yyny Jul 15 '20 at 23:22
  • @yyny Constructors and destructors aren't special, so you need to include the same (interrupted) thread in that list. You might find a way to do *all* I/O in a signal handler, but that's not a reasonable way to program. – Potatoswatter Aug 02 '20 at 09:17
  • `sleep()` and `alarm()` are not to be used together. So your example is not so good. The man pages in Linux mentions that issue. – Alexis Wilke Sep 13 '20 at 17:50
  • @AlexisWilke Right, but only because `sleep` never actually knows what woke it up. I chose this as an example only because 1) it's the only blocking function I can just throw in without adding more dummy code, and 2) reduced delay doesn't actually break a "still working" message. – Potatoswatter Sep 14 '20 at 07:35
  • If it *is* broken, for example if you find such a message getting printed at high frequency, then the solution would be to use `usleep` to keep track of time. – Potatoswatter Sep 14 '20 at 07:37
5

Move the calls to signal and alarm to just before your loop. Calling alarm over and over at high speed keeps resetting the alarm to be in one second from that point, so you never reach the end of that second!

For example:

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

void display_message(int s) {
     printf("copyit: Still working...\n" );
     alarm(1);    //for every second
     signal(SIGALRM, display_message);
}

int main(void) {
    signal(SIGALRM, display_message);
    alarm(1);
    int n = 0;
    while (1) {
        ++n;
    }
    return 0;
}
ooga
  • 14,368
  • 2
  • 18
  • 21
  • I did as you suggested and no luck.. What else could be causing this? – vivav Feb 04 '14 at 02:59
  • 1
    Any reason why you're calling signal again in display_message? Once the signal is set all thats needed is another alarm call. – tgwaste Jul 09 '17 at 05:15
  • 1
    Although many examples show `printf()` being used in signal handlers, it's not a good idea. It probably works fine in a simple program, but as it gets more complex, it is very likely to break. – Alexis Wilke Feb 11 '18 at 06:25
  • @AlexisWilke Is printf going to break because it is not reentrant or why? (showing interspersed characters is IMO not full blown breakage but inconvenience) – Paul Stelian Feb 20 '18 at 22:15
  • 1
    @PaulStelian The main problem you will notice is indeed be interspersed output. However, `printf()` may be redirected to a file system that uses a mutex/lock mechanism and another signal happen while that lock is on... and the next signal handler tries a `printf()` of its own... and fails because the second lock generates a deadlock. More info: https://docs.oracle.com/cd/E19455-01/806-5257/gen-26/index.html – Alexis Wilke Feb 21 '18 at 01:30
  • @AlexisWilke So basically yes, it's not reentrant. – Paul Stelian Feb 21 '18 at 19:42
3

Do not call alarm() twice, just call it once in main() to initiate the callback, then once in display_message(). Try this code on Linux (Debian 7.8) :

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

void display_message(int s);   //Function for alarm set up

void display_message(int s)
{
     printf("copyit: Still working...\n" );
     alarm(1);    //for every second
     signal(SIGALRM, display_message);
}

int main()
{
    signal(SIGALRM, display_message);
    alarm(1);     // Initial timeout setting

     while (1) 
     {   
          pause();
     }   
}

The result will be the following one :

copyit: Still working...
copyit: Still working...
copyit: Still working...
copyit: Still working...
copyit: Still working...
copyit: Still working...
copyit: Still working...
copyit: Still working...
copyit: Still working...
copyit: Still working...
copyit: Still working...
copyit: Still working...
Francesco Boi
  • 5,497
  • 8
  • 54
  • 83
mbornet
  • 31
  • 3
2

The alarm() call is for a one off signal.

To repeat an alarm, you have to call alarm() again each time the signal occurs.

Another issue, also, is that you're likely to get EINTR errors. Many system functions get interrupted when you receive a signal. This makes for much more complicated programming since many of the OS functions are affected.

In any event, the correct way to wait for the next SIGALRM is to use the pause() function. Something the others have not mentioned (instead they have tight loops, ugly!)

That being said, what you are trying to do would be much easier with a simple sleep() call as in:

// print a message every second (simplified version)
for(;;)
{
    printf("My Message\n");
    sleep(1);
}

and such a loop could appear in a separate thread. Then you don't need a Unix signal to implement the feat.

Note: The sleep() function is actually implemented using the same timer as the alarm() and it is clearly mentioned that you should not mix both functions in the same code.

sleep(3) may be implemented using SIGALRM; mixing calls to alarm() and sleep(3) is a bad idea.

(From Linux man alarm)

void alarm_handler(int)
{
    alarm(1);    // recurring alarm
}
    
int main(int argc, char *argv[])
{
    signal(SIGALRM, alarm_handler);
    alarm(1);

    for(;;)
    {
        printf("My Message\n");
        // ...do other work here if needed...
        pause();
    }

    // not reached (use Ctrl-C to exit)
    return 0;
}

You can create variations. For example, if you want the first message to happen after 1 second instead of immediately, move the pause() before the printf().

The "other work" comment supposes that your other work does not take more than 1 second.

It is possible to get the alarm signal on a specific thread if work is required in parallel, however, this can be complicated if any other timers are required (i.e. you can't easily share the alarm() timer with other functions.)

P.S. as mentioned by others, doing your printf() inside the signal handler is not a good idea at all.

There is another version where the alarm() is reset inside main() and the first message appears after one second and the loop runs for 60 seconds (1 minute):

void alarm_handler(int)
{
}
    
int main(int argc, char *argv[])
{
    signal(SIGALRM, alarm_handler);

    for(int seconds(0); seconds < 60; ++seconds)
    {
        alarm(1);
        // ...do other work here if needed...
        pause();
        printf("My Message\n");
    }

    // reached after 1 minute
    return 0;
}

Note that with this method, the time when the message will be printed is going to be skewed. The time to print your message is added to the clock before you restart the alarm... so it is always going to be a little over 1 second between each call. The other loop is better in that respect but it still is skewed. For a perfect (much better) timer, the poll() function is much better as you can specify when to wake up next. poll() can be used just and only with a timer. My Snap library uses that capability (look for the run() function, near the bottom of the file). In 2019. I moved that one .cpp file to the eventdispatcher library. The run() function is in the communicator.cpp file.

Alexis Wilke
  • 15,168
  • 8
  • 60
  • 116
-1

ooga is correct that you keep reloading the alarm so that it will never go off. This works. I just put a sleep in here so you don't keep stepping on yourself in the loop but you might want to substitute something more useful depending on where you are headed with this.

void display_message(int s)
{
     printf("copyit: Still working...\n" );
    // alarm(1);    //for every second
    // signal(SIGALRM, display_message);
}

int main(int argc, char *argv[])
{
    int ret;

    while(1)
    {
        signal(SIGALRM, display_message);
        alarm(1);

        if ((ret = sleep(3)) != 0)
        {
            printf("sleep was interrupted by SIGALRM\n");
        }
    }

    return (0);
}
Community
  • 1
  • 1
Duck
  • 25,228
  • 3
  • 57
  • 86
  • 4
    Calling printf from a signal handler is very unsafe. – Potatoswatter Feb 04 '14 at 04:00
  • It's not unsafe in this throw-away pgm. But fine, @vivav, don't use async-unsafe calls in a signal handler. – Duck Feb 04 '14 at 04:14
  • 1
    Really? Which is supposed to be printed first, `still working` or `sleep was interrupted`? Maybe the machine attempts to print both at the same time. – Potatoswatter Feb 04 '14 at 04:22
  • Yes, really. The handler will run and print then sleep will return with EINTR and print. Every single time since there is, for all intents and purposes, no chance of this demonstration program getting hit by another signal while executing the handler. – Duck Feb 04 '14 at 04:47
  • 1
    No, there is no guarantee that the signal handler will run on the same thread as `main` (unless POSIX forbids OS-private threads, which I doubt), nor that sleep will wait for the handler to return first. According to the POSIX spec of sleep, any signal to the process causes an immediate return. Anyway, it's just not the right way to code. – Potatoswatter Feb 04 '14 at 04:50
  • You are reaching. There is no guarantee that the handler will run on a particular thread *unless* it is the *only* thread or if you want to consider the chances of OP running on a system - near zero in my view - that spins up threads to handle a signal. We agree that putting `printf` or other non-sanctioned calls in a handler is not proper coding. But it also the easiest way to demonstrate OP's primary problem in what appears to be his first signal handling pgm without *in actuality* doing any harm. Perhaps I was remiss in not putting in the standard disclaimer but that is all. – Duck Feb 04 '14 at 05:22
  • This is wrong because the `sleep()` may be implemented using the `SIGALRM` signal. So it won't work as you think (i.e. your alarm will likely be ignored while sleeping). It will of course be implementation independent, but on Linux if you look at the man page of `alarm` there is a warning about it. – Alexis Wilke Feb 11 '18 at 06:23