-1

I'm working on a multi threaded demo app to test some program profiling tools for my company.

I spawn two threads and have a mutex to control access to a stack of objects.

I believe the top() function is returning the reference and not a copy of my object to my threads... subsequently when I release the mutex after accessing the stack the other thread grabs the mutex. Something odd is happening.

A

std::stack<polygon> * polygonList = new std::stack<polygon>();

threadedFuntion()
{
    polygon p = polygonList->top();
    polygonList->pop();
    ReleaseMutex(polyListMutex);

    // preform point in polygon check
    bool done = false;
    while(!done)
    {
       done = p.doWork();
    }
}

The program works when its structured like this

B

std::stack<polygon> * polygonList = new std::stack<polygon>();

threadedFuntion()
{
    // preform point in polygon check
    bool done = false;
    while(!done)
    {
       done = polygonList->top().doWork();
    }

    polygonList->pop();
    ReleaseMutex(polyListMutex);
}

But then its not releasing the mutex until its done doing its work.

I want the threads to grab a copy of the polygon object, take it off the stack, and release the mutex, then continue on doing its work.

EDIT I'm following this guide for mutexs and threading

EDIT Overriding operator=

polygon& polygon::operator=(const polygon &obj)
{
    if (this == &obj)
        return *this;

    std::cout << "equals";
    pa = obj.pa;

    return *this;
}
Jeffrey Haines
  • 21
  • 1
  • 5
  • `top()` does indeed return a reference. – NathanOliver Mar 20 '15 at 18:13
  • so polygon p = polygonList.top(); p is a pointer to that object? – Jeffrey Haines Mar 20 '15 at 18:14
  • 1
    No. `p` will be a copy of the top of the stack since you have it declared as a `polygon` and not a `polygon&`. Can you explain "Something odd is happening."? – NathanOliver Mar 20 '15 at 18:18
  • So my program creates a stack of polygons. The threads call p.doWork which create a random point and checks if its in the polygon. Whats strange is Code style A doesn't work but style B does. – Jeffrey Haines Mar 20 '15 at 18:35
  • it has something to do with the mutex release happenign before the work. – Jeffrey Haines Mar 20 '15 at 18:37
  • 3
    You haven't explained anything, you've just replaced "something odd" with "doesn't work". What actually happens, and what did you expect to happen? – Oktalist Mar 20 '15 at 18:41
  • I see `ReleaseMutex(polyListMutex);` but I don't see you actually locking it anywhere. Where are you locking the mutex? – Julian Mar 21 '15 at 02:46
  • Yes, `std::stack::top` returns a reference. However `polygon p = polygonList->top();` results in `p` being a copy of the top element. `I want the threads to grab a copy of the polygon object, take it off the stack, and release the mutex, then continue on doing its work.` -- That is exactly what **A** is doing. `But then it [B] is not releasing the mutex until its done doing its work.` -- In **B** you don't release the mutex until after your "preform point in polygon check" loop, so I'm not sure what you were expecting to happen; could you perhaps elaborate what is confusing to you? – Julian Mar 21 '15 at 02:51

2 Answers2

1

You haven't really given enough information, but I'm going to assume polygon::pa is some sort of pointer to an array of points, or worse, a pointer to a std::vector or some such. In your assignment operator (and presumably, your copy constructor), you are just copying the pointer, so that when the copy constructor is invoked on this line:

polygon p = polygonList->top();

... p has the same pointer as the top element in polygonList.

When you then call polygonList->pop(), the top polygon in polygonList is destroyed, which likely calls delete [] pa. Thus, p will now have a pointer to deleted memory.

You should make sure you understand the rule of three and its modern versions, rule of five and rule of zero. Once you've understood those, consider making pa a vector of points (not a pointer to a vector of points), or if you insist on taking a more manual approach, at least use a std::unique_ptr.

Also, in your example there is no reason to use std::stack<polygon> * polygonList = new std::stack<polygon>(); just say std::stack<polygon> polygonList;. It is shorter, clearer, and more correct. It looks like you are trying to program Java or C# in C++. Learn to love the stack and automatic resource management instead of manual approaches to resource management.

Community
  • 1
  • 1
Marc
  • 408
  • 3
  • 6
0

When you use top() you are getting a reference to an object on polygonList. When you use pop() you are removing that same element from polygonList, which calls that elements destructor in the process and any reference to that object is no longer valid.

In your code it looks like you are creating an object polygon and setting it equal to the reference returned from the top of the stack. I'd make sure that polygon has a proper copy-constructor that is performing a deep copy (if necessary).

When you debug it, what happens to the member variables of polygon p after you pop the top element off of your stack? That should give you some insight into what is happening.

Another thing to consider, in the "bad example" you could have multiple threads in the "do work" section of your code. Is that section of your code thread-safe? Are there other arrays or objects being accessed by that code that are common among the threads that could be causing issue?

RyanP
  • 1,898
  • 15
  • 20