Event functionality in C

Question

I am beginner to intermediate and wanted to write a little event code in c. It's one header file with 108 line pure code and it does what I was thinking it should be like for me.

EventListener.h

#ifndef EVENTLISTENER2_H
#define EVENTLISTENER2_H

#include <stdio.h>
#include <stdlib.h>

#include <pthread.h>

struct Event;

struct EventListener {
    pthread_t tid;
    struct Event *event;
    void *(*callback)(void*);
    void *arg;
    int trigger;
    struct EventListener *next;
};

typedef struct EventListener EventListener;

struct Event{
    pthread_mutex_t mtx;
    EventListener *listeners;
};

typedef struct Event Event;

void malloc_exit_on_failure(void *p)
{
    if(p == NULL)
    {
        perror("memory allocation failed");
        exit(EXIT_FAILURE);
    }
}

EventListener *EventListener_Create(void *(*callback)(void*), void *arg)
{
    EventListener *listener = (EventListener *) malloc(sizeof(*listener));
    malloc_exit_on_failure(listener);

    listener->callback = callback;
    listener->arg = arg;
    listener->tid = 0;
    listener->event = NULL;
    listener->next = NULL;
    listener->trigger = 0;

    return listener;
}

void EventListenerWait(EventListener *listener)
{
    while(listener->trigger == 0);
}

void EventListenerRun(EventListener *listener)
{
    listener->callback(listener->arg);
    listener->trigger = 0;
}

void *EventListenerThread(void *listener_arg)
{
    EventListener *listener = (EventListener*) listener_arg;

    while(1)
    {
        EventListenerWait(listener);
        EventListenerRun(listener);
    }

}

Event *Event_Create()
{
    Event *event = (Event *) malloc(sizeof(Event));
    malloc_exit_on_failure(event);

    event->listeners = NULL;

    pthread_mutex_init(&event->mtx, NULL);

    return event;
}


void Event_RegisterRaw(Event *event, void *(*callback)(void *), void *arg);

void Event_Register(Event *event, EventListener *listener)
{
    if(event->listeners == NULL)
        event->listeners = listener;

    else
    {
        struct EventListener *current = event->listeners;
        while(current->next != NULL)
            current = current->next;

        current->next = listener;
    }

    listener->event = event;

    if(pthread_create(&listener->tid, NULL, EventListenerThread, listener) != 0)
    {
        perror("cannot create thread");
        exit(EXIT_FAILURE);
    }

}

void Event_Trigger(Event *event){

    EventListener *current = event->listeners;

    while(current != NULL){
        pthread_mutex_lock(&event->mtx);
        current->trigger = 1;
        current = current->next;
        pthread_mutex_unlock(&event->mtx);
    }
}


void Event_memory_usage(Event *event)
{
    size_t size;
    int n_listeners = 0;

    size = sizeof(*event);
    
    EventListener *current = event->listeners;

    while(current != NULL)
    {
        size = size + sizeof(*current);
        current = current->next;
        n_listeners++;
    }
    printf("Event size with %d listeners: %lu bytes.\n", n_listeners, size);
}


#endif

event_test.c

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

#include "EventListener.h"

void *event_trigger_test(void *arg)
{
    printf("%s\n", (char *) arg);
}

char *arg = "Event triggered!";
char *arg2 = "Event 2 triggered!";

void *event_trigger_loop_test(void *arg)
{
    printf("invoke\n");
    while(1)
    {
        sleep(2);
        printf("i am running\n");
    }
}

int main(void)
{
    EventListener *l = EventListener_Create(event_trigger_test, arg);
    EventListener *l2 = EventListener_Create(event_trigger_test, arg2);
    EventListener *l3 = EventListener_Create(event_trigger_loop_test, NULL);

    Event *TEST_EVENT = Event_Create();

    Event_Register(TEST_EVENT, l);
    Event_Register(TEST_EVENT, l2);
    Event_Register(TEST_EVENT, l3);

    printf("EventListener Size: %lu\n", sizeof(EventListener));
    printf("Event Size: %lu\n", sizeof(Event));

    Event_memory_usage(TEST_EVENT);


    for(int i = 0; i < 5; i++) {
        sleep(i);
        Event_Trigger(TEST_EVENT);
        printf("\n"); // debug
    }

    sleep(10);

}

• What should happen to listeners from an event that are running in an infinite loop after the event is triggered?

• Is it okay/normal/best practice to create one thread per listener?

I just want to hear what you think about it and I ask for constructive criticism like improvements, hint and so on what I missed, what I should do better and if there is a better approach.

For future questions of mine I just wanted to ask if it is okay to ask for review of a header file like this tiny one or where I should be looking for that?

Answer 1

In addition to Toby's comments, here are two more issues that should be looked into.

trigger needs to be volatile

The trigger variable in your EventListener struct is currently declared without the volatile keyword. This means that the compiler is free to make assumptions about the variable when optimizations are enabled. For example, the compiler could notice that the while loop in EventListenerWait() only ever attempts to read from trigger and never writes to it, so it could incorrectly assume that it never changes.

Adding volatile instructs the compiler to always read from memory when accessing the variable, ensuring that if a different thread changes the value, the thread reading from it will get the updated value.

Also note that you really should be using a mutex lock whenever reading from or writing to any variable that is shared across threads.

EventListenerWait() blindly eats CPU time

The while loop which waits for trigger to change:

void EventListenerWait(EventListener *listener)
{
    while(listener->trigger == 0);
}

...will end up eating all of the CPU for that thread while waiting here, continuously reading the value of trigger as quickly as it can over and over again. With enough threads created, you can see how this won't scale well.

You can avoid this by adding a sleep() inside the while loop so that the thread releases time back to the OS. Sleeping one full second each iteration through the loop can begin to increase the latency before an event can start from the time it's triggered, so you might want to consider using something like nanosleep() and sleeping for 1/4th of a second or so.

Suggestion: Use pthread_cond

Ideally, you should be using the pthread_cond set of functions instead of trying to implement the event triggering yourself.

For example, calling pthread_cond_wait() is really all you would need to do inside of your EventListenerWait() function, and will automatically block your thread and release CPU time back to the OS until it is signalled. You can then trigger your event by calling pthread_cond_signal(). No trigger variable would be needed at all.

Answer 2

Files

The EventListener.h file should contain only declarations, as it needs to be safe to include in object files that are linked together. Put the definitions into a separately-compiled file (conventionally, with a .c suffix).

No need for some casts

malloc() returns a pointer to void, which is assignable to any object pointer. So there's no need to cast here:

    Event *event = (Event *) malloc(sizeof(Event));

Write that instead as

    Event *event = malloc(sizeof *event);

(Notice I apply sizeof to the expression *event - that makes it more obviously correct when the declaration of the pointer is elsewhere)

Improve the error handling

I'm not a fan of calling exit() when we're out of resources (memory, threads or anything else). Prefer to pass the failure up to the caller (e.g. by returning a null pointer) so that it can take the appropriate action.

Conserve resources

Threads aren't free, so creating a new thread for each listener is likely not to scale well.

Thread safety

Make sure users are aware that their callback can be executed by any thread - that's likely to limit what operations it can perform safely. The example demonstrates what not to do: we can't safely use printf() concurrently like that without a mutex.