Cannot cancel then join a thread using os_generic.h

[dylwhich]

Because OGCancelThread() immediately frees the thread handle (pthread_t or the Windows handle), there's no way to reliably cancel a thread and wait for it to terminate using os_generic.h. Just calling pthread_cancel() does not also join the thread, so it may continue running even after OGCancelThread returns. The pthread_cancel manpage confirms this:

After a canceled thread has terminated, a join with that thread using pthread_join(3) obtains PTHREAD_CANCELED as the thread's exit status. (Joining with a thread is the only way to know that cancelation has completed.)

The effect of this is a race condition when canceling a thread that may cause a crash or invoke undefined behavior. Threads that encounter a cancellation point frequently will usually exit by the time the statement after OGCancelThread() executes. Threads that are slower to reach a cancellation point (e.g. one with a tight loop and no function calls) may continue running after OGCancelThread() returns.

This program consistently reproduces this behavior. Invoking ./thread will run a 'slow' thread, which should almost always trip the address sanitizer; invoking ./thread f will run a 'fast' thread, which should almost always exit normally.

// Build with:
//     wget https://raw.githubusercontent.com/cntools/rawdraw/master/os_generic.h
//     cc -o thread thread.c -fsanitize=bounds-strict -fsanitize=address -O0

// Run:
// To run the 'slow thread' (almost always trips address sanitizer and errors)
//     ./thread s
//
// To run the 'fast thread' (exits successfully almost always)
//     ./thread f

#include "os_generic.h"

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

void* fastThread(void* arg)
{
    while (1)
    {
        // Do a proper sleep which should create a cancellation point
        OGUSleep(1);
        *(int*)arg = *(int*)arg + 1;
    }
}

void* slowThread(void* arg)
{
    while (1)
    {
        // Spin-wait to sleep while avoiding creating a cancellation point
        for (int i = 0; i < 100000; i++);
        *(int*)arg = *(int*)arg + 1;

        // Sleep anyway, but only after incrementing
        OGUSleep(1);
    }
}


int main(int argc, char** argv)
{
    // Check the arg, 'f' to run fast thread, otherwise slow thread
    int slow = (argc < 2 || *argv[1] != 'f');

    // Allocate a value to be used by the thread
    // The thread will increment this at the end of every loop
    int* val = malloc(sizeof(int));
    *val = 0;

    og_thread_t thread = OGCreateThread(slow ? slowThread : fastThread, val);

    printf("Running %s thread\n", slow ? "slow" : "fast");

    puts("Press Enter to cancel thread\n");

    // Wait for a character, usually doesn't happen until enter is pressed anyway
    getchar();

    // Try to cancel the thread!
    OGCancelThread(thread);

    // Save the result before we free it. Not really necessary but it's nce to see what it is.
    int result = *val;

    // Now, free the value. If the thread is still running, we'll see an invalid dereference soon!
    free(val);

    printf("Done! val = %d\n", result);

    // Sleep for 100ms, just to give address sanitizer time to print a message
    OGUSleep(100000);
    return 0;
}

[cnlohr]

OGCancelThread is also not cross-platform. For instance there is no OGCancelThread on Android. I don't know what you would like the ideal behavior to be. Do you have a preference, other than adding to the documentation "BTW there's a race condition with OGCancelThread"?

[dylwhich]

I'm not entirely sure now -- when I started writing this up I thought the correct thing to do is just join the thread after canceling it before freeing it, since that's what I wanted to do really. But I learned a bit about how cancellation actually works and now I'm not sure if that's right, since if the thread can't be canceled then the one calling OGCancelThread just ends up hanging forever. I only came across this because I was trying to figure out why some CNFA drivers don't exit cleanly, but I think that's actually better to fix with a flag and then OGJoinThread instead of relying on OGCancelThread.

[cnlohr]

Cancellation is kind of like a last-ditch effort. Cause you don't free malloc'd things. You don't close things cleanly. System resources can't be released.

Again, Android doesn't really have that primitive. Bleh, I don't know if there is a good answer.

[cnlohr]

If you do form better opinions I really want to hear it or maybe some soul will find this and comment on it in the future.

[dylwhich]

Yeah, I started looking at this from the perspective of a simple worker thread (like in CNFA) that doesn't have any resources or anything, just something like while (1) memcpy(...); -- but yeah, it turns out cancellation doesn't really work on threads like that anyway! And it seems to be the same on Windows, at least assuming this StackOverflow answer is correct:

Canceling a thread without any cooperation from the thread itself is not possible.

So, yeah, I think I'd be in favor of just adding a bit of a warning to OGCancelThread that it's not reliable and it doesn't join the thread. The one issue I do still have is that, if for some reason you do want to cancel and join a thread, there's still no way to do that through os_generic.h -- both OGCancelThread and OGJoinThread free the handle when they're done. But I don't think it would be right to always join after canceling, so just modifying the existing functions isn't the right way. And it's probably not really worth adding another function like OGCancelAndJoinThread or something since you shouldn't really be canceling threads anyway?

Either way, I know what CNFA needs to fix the issues it has with threading and it doesn't involve canceling threads at all.

[cnlohr]

I really wish I could remember why we need it. I am very tempted to remove it in the current os_generic version and hope someone screams if there's a real issue?