1

So in main I have an std::atomic<int>. It gets sent to a thread that generates random numbers:

void wind_thread(std::atomic<int>* wind)
{
    while(total_planes < max_plane )
    {
        *wind = random(1, 360);
    }
}

2 other threads get this variable and do stuff with it, here is an example of one:

void land_thread(std::atomic<int>* wind, std::queue<Plane> *land, Runway *north_south, Runway *east_west)
{
    while(total_planes <= max_plane)
    {
        if(land->size() == 0){}
        else
        {
            std::lock_guard<std::mutex> guard(my_mutex);

            std::cout << "Plane " << land->front().get_plane_id()
            << " landed on " << wind_string(wind) << " Wind was: "
            << *wind << std::endl;

            land->front().set_details(wind_string(wind));

            if((*wind >= 46 && *wind <= 135) || (*wind >= 226 && *wind <= 315))
            {
                east_west->land(land->front());
                land->pop();
            }
            else
            {
                north_south->land(land->front());
                land->pop();
            }

            total_planes++;

        }
    }
}

the wind in the land_thread is fine but when I pass it to that wind_string() function which takes an std::atomic<int>* x, it gets a new value.

It works fine if I put a lock on the wind thread, but I thought the whole point of using std::atomic was that if only one thread is writing to it, I wouldn't need to use a lock. Is it because I'm passing it to a function outside of the thread? I've been looking around online and all I can find is that I shouldn't need the lock. If someone could explain why it behaves like this or has a link that goes a bit more into how atomic behaves, it would be appreciated. Thanks.

lciamp
  • 675
  • 1
  • 9
  • 19

2 Answers2

2

Since there is no lock around atomic setting of the wind, you have absolutely no guarantee that you are going to see a value wind had at a certain time. All the atomic gives you is that as of the time of setting, this value would be propagated, so that anybody reading it before it is reset will get the set value. It also guarantees anything which is set before this atomic int is set is also going to be visible.

However, in your example, nothing prevents wind from being modified while you hold the lock in another thread.

SergeyA
  • 61,605
  • 5
  • 78
  • 137
0

1

your wind thread will use an entire thread at 100% usage just to keep your wind random. Add some sleeps there to avoid unnecessary stress on your CPU or slow downs of your program.
Or consider just generating the random value on demand. (see Why does an empty loop use so much processor time? )

2

atomic only makes the access to the data itself atomic

            if((*wind >= 46 && *wind <= 135) || (*wind >= 226 && *wind <= 315))

still can have wind changed ON EACH check. You could simply copy the current wind beforehand. Something like:

int currentWind = wind->load();
if((currentWind >= 46 && currentWind <= 135) || (currentWind >= 226 && currentWind <= 315))

Or use a mutex as you did before

Community
  • 1
  • 1
I3ck
  • 433
  • 3
  • 10
  • References vs pointers have nothing to do with the question. Also I have rarely seen anything uglier than sleep. – SergeyA Jan 04 '16 at 20:18
  • 2
    Sleep is rarely a good idea, and it isn't a good idea here. I'm not saying the code before was good, but adding sleep doesn't improve it in any real sense. – Yakk - Adam Nevraumont Jan 04 '16 at 20:18
  • @SergeyA I also provide answers to his question, so I see no problem with pointing that out – I3ck Jan 04 '16 at 20:24
  • I thought I was passing by reference in main `std::thread t2(&wind_thread, &wind);` don't I need to dereference it in the actual thread? Also, all sleep does it make it take longer to do the same thing. I tried it before I just put a lock on the thread. – lciamp Jan 04 '16 at 20:25
  • @I3ck, your first point is irrelevant (references are not any better than pointers here, and someone might even prefer pointers), and your second is just an anti-pattern. Your thirs suggestion is only going to partially help. – SergeyA Jan 04 '16 at 20:28
  • @lciamp, what else would sleep do? :) of course it will mean it would take longer :D – SergeyA Jan 04 '16 at 20:28
  • removed my pointer vs reference code and offered more than `sleep` suggestion – I3ck Jan 04 '16 at 20:33
  • Why would someone need to artifically slow itself down by sleeping for some unknown time? If a thread has the work to do, let it do it. If it does not have any work, it should wait on synchronization event until the work comes to it. – SergeyA Jan 04 '16 at 20:38
  • @SergeyA a naked `while(true)` just burns a lot of CPU http://stackoverflow.com/questions/1616392/why-does-an-empty-loop-use-so-much-processor-time . Also randomly changing the wind as fast as the CPU can handle makes no sense – I3ck Jan 04 '16 at 20:46
  • Sleep is not going to fix a design flaw. – SergeyA Jan 04 '16 at 20:48
  • Randomly changing wind based on a crude approximation of wall-clock cycles plus what the CPU can handle makes little sense in most simulations. While you have reduced CPU usage, you have still left the code in a bad state (possibly a worse one, because the entire simulation might finish during one sleep cycle: a non-changing wind). There is no question that the code before was poor; using Sleep as you have suggested also makes it poor, just poor in a different way. – Yakk - Adam Nevraumont Jan 04 '16 at 22:01
  • @Yakk can you give me some pointers on why my code is poor? This was a project I did last semester in OS class but in Java, I'm trying to learn C++11 by converting my Java programs to C++11. – lciamp Jan 05 '16 at 04:09
  • @lciamp What is the point of the wind thread? It randomly generates new values then throws them into the ether, at a rate maybe faster or maybe slower than the rate at which new planes are processed. Your land thread similarly will loop forever while `land->size()==0`, busy-waiting on undefined behavior, or never stopping. The code lacks much of a point (almost everything can be better done serially) -- the design is bad *by design*. Insofar that it isn't serial it isn't done right (you may not read from a std type while another thread writes without manual synchronization, for example). – Yakk - Adam Nevraumont Jan 05 '16 at 15:02
  • @Yakk i need the wind thread. the project works like this: 2 runways that intersect like this: +, 1 thread generates planes that either get put on a take off queue or a landing queue, all planes have to take off or land in the direction that the wind is blowing, obviously no planes can take off or land at the same time. The wind decides which runway and which direction they takeoff/land. The `land->size()==0` is so i don't try to land a plane when the land queue is empty. – lciamp Jan 05 '16 at 18:41
  • @lciamp If you need threads for bad reasons, quite possibly your code will be bad. The design you are using is a bad use of threads, so your code will be bad *by design*. If you are forced to use a bad design, your code ... will be bad. Your assignment requirements could be bad, or maybe you are interpreting your assignment requirements poorly, I cannot say. I am uninterested in solving someone's particular assignment quirks, because it will be useless to anyone who isn't laden with basically the same assignment. – Yakk - Adam Nevraumont Jan 05 '16 at 18:53
  • @Yakk the assignment was originally to be done in java. The wind was to teach us about synchronized variables and the land/take off thread was to teach us about locking/unlocking threads. I'm just trying to re-do it in C++ to learn more about C++. Maybe it's just a bad example to do in C++? I got a 100 on it in java. – lciamp Jan 05 '16 at 19:14