Is this code safe to use ?
lali...@gmail.com
{
public:
my_thread() : no_exit(true)
{ pthread_create(&thread_id, NULL, &process, static_cast
<my_thread*> (this)); }
my_thread(const my_thread&) : no_exit(false), thread_id() {}
my_thread &operator = (const my_thread&) { return *this; }
static void *process(void *oObject)
{
if (!oObject) return NULL;
my_thread *object = static_cast <my_thread*> (oObject);
object->object_process();
return NULL;
}
void object_process()
{ while (no_exit); }
~my_thread()
{
no_exit = false;
pthread_join(&thread_id, NULL);
}
private:
bool no_exit;
pthread_t thread_id;
}
In the above code the main thread is changing the no_exit variable
while the thread running object_process function continuously reads
this variable to exceute in an infinte loop.
Since no_exit is bool i guess the above code should be safe but want
to know about your comments anyway. I am just a newbie trying to learn
MT programming and i am using C++ for it.
Ian Collins
Even if it compiled, it would not be safe.
> class my_thread
> {
> public:
> my_thread() : no_exit(true)
> { pthread_create(&thread_id, NULL, &process, static_cast
> <my_thread*> (this)); }
>
Two problems here, process does not have extern "C" linkage and the
unnecessary static_cast.
> my_thread(const my_thread&) : no_exit(false), thread_id() {}
>
> my_thread &operator = (const my_thread&) { return *this; }
>
Do you really want to copy these? I don't think so.
> static void *process(void *oObject)
> {
> if (!oObject) return NULL;
> my_thread *object = static_cast <my_thread*> (oObject);
> object->object_process();
> return NULL;
> }
>
This should be a free function with extern "C" linkage.
> void object_process()
> { while (no_exit); }
>
> ~my_thread()
> {
> no_exit = false;
> pthread_join(&thread_id, NULL);
The first parameter of pthread_join is pthread_t, not pthread_t*.
> }
>
> private:
> bool no_exit;
> pthread_t thread_id;
> }
Missing semicolon.
>
>
> In the above code the main thread is changing the no_exit variable
> while the thread running object_process function continuously reads
> this variable to exceute in an infinte loop.
>
> Since no_exit is bool i guess the above code should be safe but want
> to know about your comments anyway. I am just a newbie trying to learn
> MT programming and i am using C++ for it.
No, loop may be optimised in such a way that the change will not be seen.
--
Ian Collins.
lali.cpp
> No, loop may be optimised in such a way that the change will not be seen.
Sorry, i didn't understand the above line. How is this code unsafe ?
David Schwartz
> > No, loop may be optimised in such a way that the change will not be seen.
>
> Sorry, i didn't understand the above line. How is this code unsafe ?
The compiler may optimize "while (no_exit);" to copy the value of
'no_exit' into a register and loop (forever) on that copy. The
compiler has no way to know that the value of 'no_exit' may be changed
elsewhere at the same time as this loop is running.
Languages and libraries that support multi-threaded concurrent access
to objects have visibility rules that specify how you inform the
compiler about this. The simplest way is to wrap all accesses to the
variable in a mutex.
A while loop like that would still be horrible code though, as it
would wind up spinning the CPU at its maximum speed on synchronization
code. Even better would be to use whatever thread synchronization code
your library provides for that purpose, often called an 'event'.
Since this appears to be POSIX pthreads code, you could use a
condition variable and a mutex.
So intead of:
while(my_bool);
You have this:
pthread_mutex_lock(&my_lock);
while(my_bool) pthread_cond_wait(&my_lock, &my_cond);
pthread_mutex_unlock(&my_lock);
And to change the bool, use:
pthread_mutex_lock(&my_lock);
my_bool=<whatever>;
pthread_cond_broadcast(&my_cond);
pthread_mutex_unlock(&my_lock);
This way you don't spin the CPU because the 'pthread_cond_wait' will
wait (without spinning much) for the 'pthread_cond_broadcast'.
Accesses to 'my_bool' will be safe because they're protected by the
mutex.
DS
Ian Collins
>
>
>> No, loop may be optimised in such a way that the change will not be seen.
>
> Sorry, i didn't understand the above line. How is this code unsafe ?
David provided the full explanation, but you could try it for your self.
Try adding this and compare the results building with and without
optimisation:
int main()
{
my_thread m;
sleep( 1 ); // Make sure the thread starts.
return 0;
}
--
Ian Collins.
Chris Thomasson
> class my_thread
> {
[...]
> }
>
>
> In the above code the main thread is changing the no_exit variable while
> the thread running object_process function continuously reads this
> variable to exceute in an infinte loop.
>
> Since no_exit is bool i guess the above code should be safe but want to
> know about your comments anyway. I am just a newbie trying to learn MT
> programming and i am using C++ for it.
You have a couple of issues here. Its not good a practice to create
threads in constructors because there is a possibility of a race-condition
which can result in a member function of the class being called before its
actually finished being constructed. Another problem is that you are
calling pthread_create() with an entry function that is not declared as
extern "C". Also, you are not declaring the my_thread::no_exit member
variable as volatile. This is needed in your code "as-is" because a
compiler can simply choose to optimize it away resulting in your threads
never being able to fall out of the while-loop. Of course this aspect can
be considered a poor design for all of the reasons that David Schwartz has
laid out for you; follow his advice. One more thing, IMHO it's not that
great of an idea to join threads in destructors. If the join fails for
some reason, you might want to be able to throw some type of exception. As
we know, exceptions and destructors don't go together that well at all.
IMHO, it's probably best to use "free-functions" for creating and
joining threads. I have included a very small bare-bones example program
that you can play around with which shows one very simplistic approach for
creating threads in C++:
____________________________________________________________________
/* VERY Simple Thread Library Code
______________________________________________________________*/
#include <pthread.h>
class thread_base;
extern "C" void* thread_base_entry(void*);
static void thread_create(thread_base* const);
static void thread_join(thread_base* const);
class thread_base {
pthread_t m_id;
friend void* thread_base_entry(void*);
friend void thread_create(thread_base* const);
friend void thread_join(thread_base* const);
virtual void on_thread_entry() = 0;
public:
virtual ~thread_base() throw() = 0;
};
thread_base::~thread_base() throw() {}
void* thread_base_entry(void* state) {
reinterpret_cast<thread_base*>(state)->on_thread_entry();
return 0;
}
void thread_create(thread_base* const _this) {
int const status = pthread_create(&_this->m_id, NULL,
thread_base_entry, _this);
if (status) {
throw int(status);
}
}
void thread_join(thread_base* const _this) {
int const status = pthread_join(_this->m_id, NULL);
if (status) {
throw int(status);
}
}
/* Very Simple Application Code
______________________________________________________________*/
#include <cstdio>
class my_thread : public thread_base {
public:
my_thread() {
std::printf("(%p)-my_thread::my_thread()\n",
reinterpret_cast<void*>(this));
}
~my_thread() throw() {
std::printf("(%p)-my_thread::~my_thread() throw()\n",
reinterpret_cast<void*>(this));
}
private:
void on_thread_entry() {
std::printf("void (%p)-my_thread::on_thread_entry()\n",
reinterpret_cast<void*>(this));
}
};
#define RUN_DEPTH() 10
#define THREAD_DEPTH() 32
int main() {
int runs = 0;
for (; runs < RUN_DEPTH(); ++runs) {
int i = 0;
my_thread threads[THREAD_DEPTH()];
try {
for (; i < THREAD_DEPTH(); ++i) {
thread_create(&threads[i]);
}
} catch (int const& e) {
std::printf("thread_create throws status %d!\n", e);
}
for (--i; i > -1; --i) {
try {
thread_join(&threads[i]);
} catch (int const& e) {
std::printf("thread_join throws status %d!\n", e);
}
}
}
/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
std::puts("\n\n____________________________________________\n\
Press <ENTER> to exit...");
std::getchar();
return 0;
}
____________________________________________________________________
That should compile fine. If you have any questions, fire away!
;^)
lali.cpp
Would revert soon.
Regards
lali.cpp