0

I'm trying to do a simple program that prints values using threads to test POSIX pthreads because I haven't used them before. The thing is that I know in a general way what's happening and how treads work, but I'm completely clueless about them working with pointers and references.

This is my program:

#include <cstdio>
#include <cstdlib>
#include <iostream>
#include <unistd.h>
#include <pthread.h>
#include <mutex>

std::mutex m;

void* thread1(void* n)
{   m.lock();
    
    int* number = static_cast<int*>(n);

    (*number)++;

    std::cout << "THREAD 1: " << *number << std::endl;

    sleep(1);

    m.unlock();

    return number;
}

void* thread2(void* n)
{   m.lock();
    
    int* number = static_cast<int*>(n);

    (*number)++;
    
    sleep(1);

    std::cout << "THREAD 2: " << *number << std::endl;

    m.unlock();

    return number;
}


int main(void)
{   pthread_t t1, t2;
    int i = 0;
    int* n;

    while (i < 10)
    {   pthread_create(&t1, nullptr, thread1, &i);
        pthread_create(&t2, nullptr, thread2, &i);

        pthread_join(t2, reinterpret_cast<void**>(&n));
        pthread_join(t1, reinterpret_cast<void**>(&n));

        //i = *n;
    }
}

The output should be the following:

THREAD 1: 1
THREAD 2: 2
THREAD 1: 3
THREAD 2: 4
THREAD 1: 5
THREAD 2: 6

But instead it's this one:

THREAD 1: 2050832128
THREAD 2: 1
THREAD 1: 2050832128
THREAD 2: 2
THREAD 1: 2050832128
THREAD 2: 3
(THREAD 1 repeats a random number for ever
but THREAD 2 does what it should.)

I know it has to do with the way I'm managing my variables, but don't know how to fix it.

EDIT 1: Added some Mutex functionalities and commented i = *n to see how the numbers came out.

EDIT 2: Changed *number++ to (*number)++ in void* thread1(void*) and now everything works properly.

All the information about Mutex you need to understand this post is in this other post.

Simeón
  • 29
  • 7
  • 2
    When you do `int* n;`, what does `n` point to? – NathanOliver Jan 11 '21 at 19:26
  • Initially nowhere. That variable is where the return value of the functions will be stored with 'pthread_join'. – Simeón Jan 11 '21 at 19:28
  • 2
    I'm not very knowledgeable regarding pthread but you are casting a pointer with one level of indirection (`int*`) to a pointer with two levels of indirection (`void**`) which is a huge red flag to me. Edit : My guess would be that `reinterpret_cast(n)` should just be `&n`. – François Andrieux Jan 11 '21 at 19:29
  • @HTNW Nope, same result, but thanks ^^ – Simeón Jan 11 '21 at 19:30
  • 3
    Your threads are both accessing `i` for write without synchronization. Your code has undefined behavior, even if those sleeps make it seem like it has the expected behavior. – François Andrieux Jan 11 '21 at 19:31
  • @FrançoisAndrieux Yeah, it makes sense, but how should I access the value pointed by n? – Simeón Jan 11 '21 at 19:31
  • 2
    You have two problems with the code, one more subtle than another. The not-so-subtle one is that you pass a pointer to the pointer to the integer to thread, but treat it as if it was a pointer to integer (mismatch in levels of indirection). The subtle issue is un-synscronized access to non-atomic variables from two different threads. – SergeyA Jan 11 '21 at 19:33
  • 1
    @Simeón You need to introduce synchronization, usually with a `mutex` object, to make sure the threads aren't using the variable at the same time. Multithreading is one of the most difficult things to get right in C++ and it cannot be adequately explained in a Q/A post, much less a comment. You should find a reputable book on the subject to get started. – François Andrieux Jan 11 '21 at 19:33
  • 1
    On top of everything else, `*number++;` *certainly* isn't doing what you think. That doesn't increment the value pointed-to, it increments the pointer after retrieving the current value, used in a dereference operation and the result of which never actually used for *anything*. – WhozCraig Jan 11 '21 at 19:38
  • 1
    fwiw, with `std::thread` you can have have same trouble but in contrast to bare pthreads you dont have to. – 463035818_is_not_an_ai Jan 11 '21 at 19:46

1 Answers1

4

Your immediate issue lies in these lines:

pthread_join(t2, reinterpret_cast<void**>(n));
pthread_join(t1, reinterpret_cast<void**>(n));

The pthreads library will dereference the pointer you pass to pthread_join and modify the pointed-to pointer to point to the thread function's return value. n doesn't point to a pointer (or anything, for that matter) though, and so things go off the rails.

Instead of passing n to pthread_join, you need to pass a a pointer to n:

pthread_join(t2, reinterpret_cast<void**>(&n));
pthread_join(t1, reinterpret_cast<void**>(&n));
                                       // ^---- Note the extra &

After fixing that issue you'll find a couple of others. As noted in the comments to the OP, *number++ increments the pointer number, dereferences the (invalid) pointer and then immediately discards the result. It does not increment the value pointed to by number as you likely intend. For that you want (*number)++.

Your code also has a data race since there's no synchronization between your two threads and they both try to modify i. You'll need some sort of synchronization (i.e. a mutex or atomic variable) to make your program's behavior well-defined.


Note also that pthreads is a C interface. You'll find it doesn't play well with lots of C++ features. You'll likely want to use std::thread for most C++ applications.

Miles Budnek
  • 28,216
  • 2
  • 35
  • 52
  • @FrançoisAndrieux `&n` is an address of `n`, which points to an int. Everything is OK. Using a pointer with the name `n` is a bad idea, as it makes the code harder to read. – zkoza Jan 11 '21 at 19:43
  • Data race is evident, but it can be "fooled" by using different time intervals in each `sleep`. Not kosher, incorrect, but may make OP happy for a while. Of course a program with a race condition is incorrect, so the next thing OP should do is to learn about data races and how to deal with them in pthreads. – zkoza Jan 11 '21 at 19:47