removing element from deque from different threads results in segmentation fault

[kushal bhattacharya]

hi,
The issue i am facing is i am trying to remove element from deque when a certain condition is met from different threads and i enlcosing the whole thing in lock.it works for some steps but after that when i removing elements in later steps i get segmentation fault .What could be the probable reasons for this kind of issues ?
Thanks,
Kushal

[kushal bhattacharya]

This part of the code leads to seg fault
for (auto it = msgContnr->begin(); it != msgContnr->end();) {
if ((it->get()->get_opcode() == PUBLISH) && (it->get()->get_msg_id() == msg_id) &&
(it->get()->get_sockfd() == new_fd)) {
it = msgContnr->erase(it);
// break;
}
}

[kushal bhattacharya]

Here i am pushing some object into the deque and then according to the condition met i am erasing that element through the iterator.In some post in stackoverflow i read that this may be due to some invalid referencing but how do i use it correctly then ?

[kushal bhattacharya]

To be more clear i am doing his operation from differen threads and thus the deque is accessible to all the threads running simultaneously

[kushal bhattacharya]

hi,
i read this post in stackoverflow but i really couldnt really follow why i would get invalid iterator
http://stackoverflow.com/questions/2874441/deleting-elements-from-stl-set-while-iterating

[Paavo Helde]

You have a bug in your multithread locking code. For example, a lock
might be missing in the code part which reads the deque.

[Bonita Montero]

I'm not sure if I'm right, but afaik iterators are invalidated when you
erase() on a container.

--
http://facebook.com/bonita.montero/

[David Brown]

I believe you are correct there.

Certainly it is rarely a good idea to modify the variable you are
looping over from within the loop.

[kushal bhattacharya]

actually i am using condition variable for notifying when there is any modification or addition done on the dequeue and on the reading side i am waiting by the condition variable to see if the deque is not emptyand accordingly within that lock on the condtion variable i am reading from the deque after i extracted the element from the deque i unlock the lock passed on the waiting condition variable

[kushal bhattacharya]

Hi,
Sorry if i didn't maintain any indentation or
any posting rules as such but from now on I am
trying to post it in a well formatted way.
Thanks,
Kushal

[Paavo Helde]

In this particular code snippet there is nothing suspicious, there are
no iterators whose values are preserved over the erase() call.

I bet the problems are in the code the OP has not shown.

[Chris M. Thomasson]

On 3/17/2017 3:13 AM, kushal bhattacharya wrote:
> To be more clear i am doing his operation from differen threads and thus the deque is accessible to all the threads running simultaneously
>

How are you synchronizing access to the deque?

[kushal bhattacharya]

Hi,
Actually the code has several files .
If you want i can mail you all the files.
I can't really figure out which part exactly is the suspicious one.
I am synchronsing access using condtion variables notifying waiting thread when any modification is done on the deque and on the waiting thread i use that same condition variable with lock on it.

[kushal bhattacharya]

Hi,
Could you guys help me how do i detect the problematic area and post here?

[kushal bhattacharya]

When i try to remove a non existent element does it also cause undefined behaviour?

[Alf P. Steinbach]

On 18-Mar-17 6:03 AM, kushal bhattacharya wrote:
> Hi, Could you guys help me how do i detect the problematic area and
> post here?

Well, you have a bug somewhere, apparently (but that's only your
impression) connected to removal of items from a deque by multiple threads.

To fix a problem it's important to be able to reproduce it consistently,
but thread synchronization problems are notoriously difficult to reproduce.

So I suggest that you /isolate/ the deque. Place it as a private member
of a class. Define access methods that always lock it via mutex.

If the problem persists, then /if/ it's connected to the deque it's
probably because some thread retains a reference to something in the deque.

This is more difficult to hunt down, but the solution then is probably a
bit of indirection, to deal with the real shared ownership
(contradicting the assumption of central ownership in the deque). One
approach then is to not let your wrapper hand out raw references or
pointers to data in the deque. Hand out shared_ptr's.

And you can do the same with the items as with the deque: isolate them,
by placing them in thread safe wrappers.

You can also attack this from the opposite end.

Namely, try to build up a simplest possible example that exhibits the
ungood behavior.

In the end, when no reasonable approach has worked, if that should
happen, then just find some totally different way of doing whatever it
is that needs to be done.


Cheers & hth.,

- Alf

[kushal bhattacharya]

Hi Alf,
I am globally accessing the deque in different classes instances so thats why i have kept it globally.Is it a wise choice or do I have
to wrap it within some class?

[Ian Collins]

Please try and reply (with context) to the message you are responding to
rather than keep replying to yourself! That will make it easier for
people to help you..

--
Ian

[kushal bhattacharya]

I apologise if i have been wrong actually i was trying to be more clear .Actually now i am getting seg fault in my manipulation of deque

[Christian Gollwitzer]

Am 18.03.17 um 06:59 schrieb Alf P. Steinbach:

> On 18-Mar-17 6:03 AM, kushal bhattacharya wrote:
>> Hi, Could you guys help me how do i detect the problematic area and
>> post here?
>
> Well, you have a bug somewhere, apparently (but that's only your
> impression) connected to removal of items from a deque by multiple threads.
>
> To fix a problem it's important to be able to reproduce it consistently,
> but thread synchronization problems are notoriously difficult to reproduce.

Fortunately, there are tools which can help. If you can run your program
on Linux, you can use Valgrind to check it for memory errors and
Helgrind to find synchronization errors. These are two of the best
debugging tools I've ever used.

Christian

[Paavo Helde]

On 18.03.2017 7:03, kushal bhattacharya wrote:
> Hi,
> Could you guys help me how do i detect the problematic area and post here?

Multithreading bugs are difficult to debug. One simple approach is to
inspect the code carefully to verify that the multithread locking code
is correct. This is very simple: do a text search over all your files
for the name of the global deque. Go through the *ALL* locations where
the deque is accessed and verify that there is the proper lock variable
defined in an enclosing scope (and that it is not a declaration of a
function, that it is locking the same global mutex, etc.)

And please learn to reply in the correct subthread and to quote the
relevant context! Using a proper newsreader like Thunderbird instead of
Google Groups might be of some help.

[Jerry Stuckle]

On 3/18/2017 3:50 AM, kushal bhattacharya wrote:
> I apologise if i have been wrong actually i was trying to be more clear .Actually now i am getting seg fault in my manipulation of deque
>

You can't just lock the deque during the removal process. Removing an
element can cause any pointer or iterator in other threads to become
invalid. This will cause undefined results (including potential segfaults).

--
==================
Remove the "x" from my email address
Jerry Stuckle
jstu...@attglobal.net
==================

[kushal bhattacharya]

hi,
So what should I do in this case?

[Paavo Helde]

On 18.03.2017 20:51, kushal bhattacharya wrote:
> On Saturday, March 18, 2017 at 8:01:42 PM UTC+5:30, Jerry Stuckle wrote:
>> On 3/18/2017 3:50 AM, kushal bhattacharya wrote:
>>> I apologise if i have been wrong actually i was trying to be more clear .Actually now i am getting seg fault in my manipulation of deque
>>>
>>
>> You can't just lock the deque during the removal process. Removing an
>> element can cause any pointer or iterator in other threads to become
>> invalid. This will cause undefined results (including potential segfaults).
>

> So what should I do in this case?
>

It's simple, do not keep or use any pointer or iterator values after
releasing the lock.

So far you have not shown to us any code example which actually performs
the locking. It might be that your locking code just does not do what
you think it is doing. Post some more complete examples of how you read
and modify the deque.

[Öö Tiib]

On Saturday, 18 March 2017 20:51:58 UTC+2, kushal bhattacharya wrote:
> On Saturday, March 18, 2017 at 8:01:42 PM UTC+5:30, Jerry Stuckle wrote:
> > On 3/18/2017 3:50 AM, kushal bhattacharya wrote:
> > > I apologise if i have been wrong actually i was trying to be more clear .Actually now i am getting seg fault in my manipulation of deque
> > >
> >
> > You can't just lock the deque during the removal process. Removing an
> > element can cause any pointer or iterator in other threads to become
> > invalid. This will cause undefined results (including potential segfaults).
> >
>

> hi,
> So what should I do in this case?

How can we know? We do not know your code and there are endless ways how
to write code that manipulates containers.

If you don't want references to deque invalidated then push_front,
push_back, emplace_front and emplace_back do not invalidate any references
to elements of the deque. Also when erasing at either end of the deque,
references to non-erased elements are not invalidated by erase, pop_front
and pop_back. Also you can just avoid having any references to container
elements stored anywhere once you released a lock to it.

[kushal bhattacharya]

{
lock_guard<mutex> lock_guard1(m2);
std::find_if(msg_lists.begin(), msg_lists.end(),
[&](std::pair<int, shared_ptr<msg_list>> msg_obj)mutable {
if ((msg_obj.second->get_opcode() == PUBREC) &&
(msg_obj.second->get_msg_id() == msg_id) &&
(msg_obj.second->get_sockfd() == sock_fd)) {
// {
// lock_guard<mutex> lock_guard1(m2);
msg_obj.second->set_opcode(PUBCOMP);
msg_obj.second->set_sentflg(false);
cout << "MSG LIST UPDATED in PUBREL"
<< ((msg_obj.second->get_opcode() == PUBCOMP) &&
(msg_obj.second->get_msg_id() == msg_id) &&
(msg_obj.second->get_sockfd() == sock_fd)) << endl;
data_cond.notify_one();
// }
}
return ((msg_obj.second->get_opcode() == PUBCOMP) &&
(msg_obj.second->get_msg_id() == msg_id) &&
(msg_obj.second->get_sockfd() == sock_fd));
});
}



This is the part where i push the element into the container

[kushal bhattacharya]

On Friday, March 17, 2017 at 1:22:51 PM UTC+5:30, kushal bhattacharya wrote:
> hi,
> The issue i am facing is i am trying to remove element from deque when a certain condition is met from different threads and i enlcosing the whole thing in lock.it works for some steps but after that when i removing elements in later steps i get segmentation fault .What could be the probable reasons for this kind of issues ?
> Thanks,
> Kushal


{
lock_guard<mutex> lock_guard1(m2);

msg_id = pars_obj->get_msgid();
shared_ptr<msg_list> msg_obj(new msg_list(lastinsrt_id, sock_fd, PUBACK));
// opcode = PUBACK;
msg_obj->set_msg_id(msg_id);
topic_name = pars_obj->get_topic();
msg_obj->set_topic(topic_name);
//tmp_msg_lists =&msg_lists;
// msg_Contnr *tmp_msg_lists=&msg_lists;
// msg_lists.push_back(msg_obj);
msg_lists[msg_counter] = msg_obj;
msg_counter++;
data_cond.notify_one();
}

} else if (qos == 2) {
// int msg_idLSB = pblshpacket.msgid_Lsb;
// int msg_idMSB = pblshpacket.msgid_Msb;

{
lock_guard<mutex> lock_guard1(m2);
msg_id = pars_obj->get_msgid();
shared_ptr<msg_list> msg_obj(new msg_list(lastinsrt_id, sock_fd, PUBREC));
// opcode = PUBREC;
msg_obj->set_msg_id(msg_id);
topic_name = pars_obj->get_topic();
msg_obj->set_topic(topic_name);
msg_obj->set_opcode(PUBREC);
// msg_Contnr *tmp_msg_lists=&msg_lists;
// msg_lists.push_back(msg_obj);
msg_lists[msg_counter] = msg_obj;
msg_counter++;
data_cond.notify_one();
}
// }
// data_cond.notify_one();
}
}

[Chris M. Thomasson]

Have you ever tried ThreadSanitizer?

https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual

Or Relacy?

http://www.1024cores.net/home/relacy-race-detector

Imvho, Relacy is pretty nice, dedicated to checking C++ memory model,
wrt the primitives it offers. It can help one model their apps sync
scheme. Have not tried ThreadSanitizer yet.

[kushal bhattacharya]

I will give it a shot then thanks man

[Paavo Helde]

On 18.03.2017 21:21, kushal bhattacharya wrote:
> On Friday, March 17, 2017 at 1:22:51 PM UTC+5:30, kushal bhattacharya wrote:
>> hi,
>> The issue i am facing is i am trying to remove element from deque when a certain condition is met from different threads and i enlcosing the whole thing in lock.it works for some steps but after that when i removing elements in later steps i get segmentation fault .What could be the probable reasons for this kind of issues ?
>> Thanks,
>> Kushal
>
> {
> lock_guard<mutex> lock_guard1(m2);
> std::find_if(msg_lists.begin(), msg_lists.end(),
> [&](std::pair<int, shared_ptr<msg_list>> msg_obj)mutable {

msg_lists does not look like a std::deque to me. Probably this is
std::map or something instead. Not that it would make much difference,
but so far you have been claiming it is a deque.

Locking seems OK, assuming that m2 is defined in the correct place.

One normally uses the return value of std::find_if() instead of doing
modifications on the fly. Currently your std::find_if() is not finding a
PUBREC item in the container if there is a matching PUBCOMP item present
before that, not sure if this is intended or not. Separating the find
and modification actions would clear that up.

Your usage of the std:: prefix seems very inconsistent. Suggesting to
use it all the time.

Could you show also the code parts which are *reading* the container,
together with the locking. Seeing the container and mutex declarations
and function signatures would not hurt either.

[Jerry Stuckle]

On 3/18/2017 2:51 PM, kushal bhattacharya wrote:
> On Saturday, March 18, 2017 at 8:01:42 PM UTC+5:30, Jerry Stuckle wrote:
>> On 3/18/2017 3:50 AM, kushal bhattacharya wrote:
>>> I apologise if i have been wrong actually i was trying to be more clear .Actually now i am getting seg fault in my manipulation of deque
>>>
>>
>> You can't just lock the deque during the removal process. Removing an
>> element can cause any pointer or iterator in other threads to become
>> invalid. This will cause undefined results (including potential segfaults).
>>
>

> hi,
> So what should I do in this case?
>

Fix your code. No one can tell you how to fix code you've never shared.

[Chris M. Thomasson]

On 3/17/2017 12:52 AM, kushal bhattacharya wrote:
> hi,
> The issue i am facing is i am trying to remove element from deque when a
> certain condition is met from different threads and i enlcosing the whole
> thing in lock.it works for some steps but after that when i removing elements
> in later steps i get segmentation fault .What could be the probable reasons
> for this kind of issues ?

Fwiw, here is a quick and dirty little program I wrote that uses a c++
deque as a dynamic queue for a single producer, multiple consumer setup.
Here is the code:
_____________________________________
#include <cstdio>
#include <deque>
#include <condition_variable>
#include <mutex>
#include <memory>
#include <string>
#include <thread>
#include <algorithm>
#include <cassert>


template<typename T>
struct queue
{
typedef std::deque<T> raw_queue_t;

raw_queue_t m_queue;
std::condition_variable m_cond;
std::mutex m_mutex;

void push(T const& obj)
{
{
std::unique_lock<std::mutex> lock(m_mutex);
m_queue.push_back(obj);
}

m_cond.notify_one();
}

T pop()
{
T front;

{
std::unique_lock<std::mutex> lock(m_mutex);
while (! m_queue.size()) m_cond.wait(lock);
front = m_queue.front();
m_queue.pop_front();
}

return front;
}
};


typedef queue<unsigned int> string_queue_t;
#define CONSUMERS 5
#define N 1000


void producer(string_queue_t& queue)
{
std::printf("producer::queue::(%p) - enter\n", (void*)&queue);

for (unsigned int i = 0; i < N; ++i)
{
queue.push(i + 1);
std::this_thread::yield(); // just for some spice
}

for (unsigned int i = 0; i < CONSUMERS; ++i)
{
queue.push(0);
std::this_thread::yield(); // just for some spice
}

std::printf("producer::queue::(%p) - exit\n", (void*)&queue);
}


void consumer(unsigned int id, string_queue_t& queue)
{
std::printf("consumer(%u)::queue::(%p) - enter\n", id, (void*)&queue);

unsigned int prev = 0;

for (;;)
{
unsigned int msg = queue.pop();

std::printf("consumer(%u)::msg::(%u)\n", id, msg);

if (msg == 0) break;

assert(msg > prev); // validate fifo nature

prev = msg;
}

std::printf("consumer::queue::(%p) - exit\n", (void*)&queue);
}


int main(void)
{
{
string_queue_t queue;

std::thread consumers[CONSUMERS];

for (unsigned int i = 0; i < CONSUMERS; ++i)
{
consumers[i] = std::thread(consumer, i, std::ref(queue));
}

std::thread producer_thread(producer, std::ref(queue));

producer_thread.join();

for (unsigned int i = 0; i < CONSUMERS; ++i)
{
consumers[i].join();
}
}

std::printf("\nComplete, hit <ENTER> to exit...\n");
std::fflush(stdout);
std::getchar();

return 0;
}
_____________________________________


Can you run it?