C++ - Using STL queue dumps core

[pankaj.talk]

Hi All,

I am having a C++ application which loads a dynamic library (.so on UNIX and .dll on Windows). In the dynamic library, I have two threads. One is the reader and the other is the writer.

The writer accepts data from the C++ application and puts it onto a STL queue using push. The reader thread reads data from the queue using front and pop. However, the C++ application dumps a core after running for around one day.

The data thats written onto the queue is a class object. The class is

class OBJ
{

public:
OBJ() {};

~OBJ() {};

OBJ( char * name, int type, char * value) {
this->countername = name;
this->countertype = type;
this->countervalue = value;
}

const char * getCounterName() {
return A.c_str();
}

int getCounterType() {
return B;
}

const char * getCounterValue() {
return C.c_str();
}


protected:

string A;
int B;
string C;

};

The writer thread calls the following to push data:

push( OBJ(szXYZ, idata, szMNO ));

The reader thread calls the following to read and pop data:

OBJ obj;

if ( (size = QUEUE.size()) > 0 ) {

obj = QUEUE.front();
QUEUE.pop();

}

Is anything wrong with this? I see the core dump after about a day on Solaris 8 SPARC.

Thanks
Pankaj

[GNiewerth]

You have to make the queue access thread safe. Crashing after about one day is pretty random, it might crash after 5 minutes as well as after 5 months.
Make sure inserting and popping an element is encapsulated in a critical section (or some other kind of synchronization object). Using a critical section makes sure that the current operation will be completed without being interrupted by another thread.

PS:
Please use code tags to format your code

[JohnW@Wessex]

This may also help, if you don't mind coding your own queue.

http://www.ddj.com/cpp/210604448

[pankaj.talk]

Thanks for the suggestions.

I forgot to mention this. We are using critical sections to ensure that the push, front and pop calls are all atomic.

You all may see that the OBJ class doesn't have the copy constructor and overloaded assignment operator defined. Do you think this might be causing corruption by any chance? The compiler would be generating the defaults ones and i think these should be sufficient in this case.

Any other pointers?

[JohnW@Wessex]

Are A, B & C supposed to be countername, countertype & countervalue?

[Paul McKenzie]

Thanks for the suggestions.

I forgot to mention this. We are using critical sections to ensure that the push, front and pop calls are all atomic.

Your usage of this class is what's most important, which you didn't show.

For example, we don't know what you're doing with those return values for getCounterName(), getCounterValue(). Why didn't you just return std::string instead of the somewhat suspicious const char *? That would remove the suspicion from these functions. Currently those functions return const char*, and we don't know what you are doing with those values.

You also construct OBJ objects using variables we have no idea where they came from.

Then you also have this:

OBJ( char * name, int type, char * value) {
this->countername = name;
this->countertype = type;
this->countervalue = value;
}

Why not this?

OBJ( char * name, int type, char * value)  : 
             countername(name),
             countertype(type), 
             countervalue(value) { }

Not using initialization as I did above could indicate that the rest of the code may be written in a faulty manner (even though the assignments were not faulty, but leaves a bad perception as to the robustness of the rest of the code).

Points:

1) Because you say you've ensured that you have placed critical sections really doesn't prove much to us reading the code. We have no idea if the critical sections were implemented correctly, as all we have is your word that it's done correctly. We won't know until we see the code ourselves. This not only will confirm to us it's correct, we won't waste time going after things that have nothing to do with the problem.

2) If we took your code and placed it in a simple app, I bet it doesn't have any problems. This would mean that the problem is the actual usage of the classes themselves. Just focusing on your class as the problem is the wrong approach at this point.

You all may see that the OBJ class doesn't have the copy constructor and overloaded assignment operator defined. Do you think this might be causing corruption by any chance?

There is no corruption. The types are int's and string's, which are all copyable without any further intervention.

To sum up, there is nothing wrong with that OBJ class itself, and you should shift your focus to your application and how you're using these OBJ objects.

Regards,

Paul McKenzie

[NMTop40]

I have written my own message queues (prodcon_queue) for multi-threaded systems but it is fairly complex. I had to write a mutex wrapper and a condition variable wrapper.

boost may have one though that suits your needs. thread-safe collections is something they have added in the last few years.

As for your OBJ class, there are things in it that could be improved but it looks like a prototype anyway as you have called your members A, B and C and then in your constructor you gave them real names so I guess you are copying here a subset of what you are actually doing.

Please make your code const-correct too.

Beware of returning const char * from your get methods as you have used c_str(). These pointers only have the lifetime of the strings they come from and then only if the strings are not modified at any time. Unless you know that it is performance critical then it is better to return a std::string. Actually I have a portable_string class that uses reference-counting for long strings (longer than 16 characters) and is immutable other than assignment. The reference-counting can be made atomic so is thread-safe.