unit tests for ROS Thread Obj

[sarimabbas]

made a new branch because the other one started to get messy

for now, i used a mutex to lock and unlock the thread functions as needed to address the concurrency issue. hopefully when switching to shared-ptr with std::threads it may not be required.

[alecive]

Good work. I think we can merge this as it is.

[alecive]

I'll do some minor (cosmetic) changes, but I like what I see 👍

[alecive]

Actually, after reviewing the code I am thinking that it can drastically simplified by implementing everything inside the ROSThreadObjTest class object. This is what I would do:

• Move every function into the class object.
• void add_function(void *(* func)(void *)) is going to be moved into void add_function(const std::string& func_name) and then you add the function with a switch or an if
• any function signature will be void function_name(), since now you can directly access the internal variable without passing it as an argument

Other things (that now will not be important since you'll change them):

• All of this static_cast to int* does not make much sense. Why don't you just access the variable as it is?
• The usage of (void* var) into function signature is allowed and perfectly valid but a bit obscure. So, in the future please prefer putting the actual variable type as input, it makes everything more clear to anybody who will look at the code.
• Other thing that I don't understand is the use of a mutex. A shared_ptr object should automatically take care of concurrent read/write operations on a variable isn't it? So if that is the case, why do you have to lock/unlock the pointer in order to achieve thread safety?

[alecive]

See my comment for the changes I would like you to implement.

[sarimabbas]

Okie, just making the changes. As for the concerns you brought up, the first and third are related. I was trying to pass the shared_ptr as an argument to the thread functions. To do this, I had to convert to and from void* because pthread_create accepts a void* argument. Consequently, the thread safety benefits of the shared_ptr were lost because the conversions were being done with the raw_ptr enclosed in it. However, it looks like this may not be an issue with your approach

[alecive]

Ah okay, that is correct then..but as you said, it will be fixed by my suggestions :)

[sarimabbas]

Yup 😊 But I'm running into a little bit of trouble. I've moved the functions into the class. However, class member functions have a type void*(ROSThreadObjTest::*)(void*) instead of void*(*)(void*). This means that the threads cannot start because pthread_create expects the latter type. One way to resolve this would be to make all member thread functions static, giving them the same type as the latter. But this is not useful because then we cannot create shared_ptr instances within a static member function. Would you have any advice for this? I'll continue to try and find a solution

edit: one way could be like in robot_interface.cpp, where you created a wrapper ThreadEntryFunc that accesses an object/instance's ThreadEntry method. But doing this here would mean creating a separate wrapper for each thread function.

[alecive]

I would do like in robot interface class. It seems the better option any way

[sarimabbas]

Codacy is complaining because of the C-style syntax needed to implement the wrapper approach. I am not entirely sure why one of the unit tests is failing on Travis, I'll look into it

edit: I believe this is because the shared_ptr is partially thread-safe. Its control block portion is thread-safe, but the resource it manages (the int var) is not thread-safe.

[alecive]

For the unit test, it's because it is probably not thread safe:

<failure message="Value of: rtot3.var_value()&#x0A;  Actual: 246&#x0A;Expected: 250" type=""><![CDATA[/home/travis/catkin_ws/src/baxter_collaboration/baxter_collaboration_lib/test/test_ros_thread_obj.cpp:291

Value of: rtot3.var_value()

  Actual: 246

Expected: 250]]></failure>

For the shared_ptr, interesting. At this point, you don't even need it. Just place a setVar method and getVar method with a lock_guard to a mutex.

For codacy, it is right, but we don't care 😎

[sarimabbas]

I've been puzzling through this get/set + lockguard approach for a while. If we keep the lock guard within just the get/set methods, it causes some problems.

int get_var()
{
    std::lock_guard<std::mutex> lk(mtx);
    return var;
}
void set_var(int arg)
{
    std::lock_guard<std::mutex> lk(mtx);
    var = arg
}

For e.g: if 2 threads run that loop to increment var, then we might have a situation where: T1: get 0. T2 : get 0. T1: set 1. T2 : set 1. The get and set methods become thread safe, but not a sequence of get and set methods.

I think the lockguards may be needed within the thread functions themselves. For e.g this works:

void slow_hundred_adder()
 {
    for(int i = 0; i < 100; i++)
    {
        std::lock_guard<std::mutex> lk(mtx);
        var = var + 1;
        sleep(0.1);
    }
}

What do you think?

[alecive]

I think that your solution is okay. A better solution would be to get rid of the set and just create an addOne method 😎

[sarimabbas]

Ok added the safe methods 😄

[alecive]

@sarimabbas See my commits and try to understand what I did in order to make this class with a similar code style as the rest of the repo. I did some other medium severity changes, which should be pretty straightforward to understand (please report to me if you don't).

The only thing that it's worth mentioning here is that in tenMultiplier and tenDivider you don't place sleeps when you concurrently access the variable, but rather only inside the thread execution. Whilst this can be good anyway as it may create some minor issues related to concurrency, I decided to remove the sleeps altogether since they were not placed where the concurrency is (see commit 6ce9ded ). There is no simple fix to this, as you cannot incrementally divide as you do with tenAdder for example.

[alecive]

Ok merging! Good job @sarimabbas

Now we can move to std::threads (see #43 ).

[sarimabbas]

I understand, and thank you for the edits. I'll get started on the std::threads