8
\$\begingroup\$

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?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Note that %lu might not be the correct format specifier for size_t (returned by sizeof). Use %zu instead. See also: stackoverflow.com/q/2125845/20017547 \$\endgroup\$
    – Harith
    Jan 27, 2023 at 18:59
  • \$\begingroup\$ Is it intentional that you return nothing from callback functions whose return type is void*, not void? Should these return NULL? Are there other callbacks that are supposed to return a pointer to some static data structure? \$\endgroup\$
    – Davislor
    Jan 28, 2023 at 2:26
  • \$\begingroup\$ Yeah... no, i am not sure how to handle that. Maybe adding a return member to the listener struct or just hand this to the user. \$\endgroup\$ Jan 28, 2023 at 15:40

2 Answers 2

4
\$\begingroup\$

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.

\$\endgroup\$
3
  • 3
    \$\begingroup\$ Note that if you properly use mutexes, you don't need volatile. \$\endgroup\$
    – G. Sliepen
    Jan 28, 2023 at 10:12
  • \$\begingroup\$ @G.Sliepen What does have correct usage of mutexes to do with not having to use volatile? volatile simply tells the compiler to not optimize variable accesses. How are the two related? \$\endgroup\$
    – Marco
    Jan 28, 2023 at 13:38
  • \$\begingroup\$ @Marco Locking a mutex in some way has to add a barrier as well that will prevent loads from and stores to a variable protected by the mutex from being moved outside the region where the mutex is locked. So if you use mutexes, you don't need volatile for the variables guarded by the mutex. \$\endgroup\$
    – G. Sliepen
    Jan 28, 2023 at 13:47
8
\$\begingroup\$

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.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ It is safe to call printf() from multiple threads (ie, it will not crash or cause stdio to get in a bad state), but the question is whether the output is as if each call to printf() was atomic. See this StackOverflow question for more information. \$\endgroup\$
    – G. Sliepen
    Jan 27, 2023 at 15:40
  • \$\begingroup\$ First of all thanks for taking time. What else should I do? Is it good to let every callback run after the other without a thread? What if multiple things should be executed on some event at the same time? Aren't threads the only way to achive this? I read about threads on stackoverflow and one said that the creation of threads is most expensive. Do you mean it like this? They suggested to use a threadpool, would that be better? Otherwise I dont know how to realize paralelism with events. \$\endgroup\$ Jan 27, 2023 at 17:13
  • 2
    \$\begingroup\$ @mortytheshorty Yes, a thread pool is typically used for cases like this. You can simply precreate a pool of (for example) eight threads at initialization time, then allow your events to run on an available thread in the pool. When your event ends, the thread returns back to the pool to be used again. If there happen to be no available threads in the pool, then the event should be added to a queue to process later once a thread has become available again. \$\endgroup\$
    – falken
    Jan 28, 2023 at 8:44

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.