#include <new>
#include <string>
#include <windows.h>
template <class Object>
class ILink
{
// get rid of this soon...
#define IDENTIFIER std::string(typeid(*this).name())
private:
unsigned long *_count;
Object *_reference;
CRITICAL_SECTION *_critical;
void _detach (void);
public:
ILink (Object* reference=0);
ILink (const ILink<Object>& copy);
~ILink (void);
ILink<Object>& operator = (const ILink<Object>& assign);
Object& operator * (void);
const Object& operator * (void)const;
bool valid (void)const;
bool operator ! (void)const;
bool operator == (const ILink<Object>& link)const;
bool operator != (const ILink<Object>& link)const;
};
template <class Object>
inline void ILink<Object>::_detach(void)
//
// Decrease the objects reference counter
// by one. If the counter has reached zero
// delete both the object and it's counter.
// For multi-threading purposes a critial
// section is used to avoid double deletion.
//
{
EnterCriticalSection(_critical);
--(*_count);
switch (*_count)
{
case 0:
delete _reference;
delete _count;
LeaveCriticalSection(_critical);
DeleteCriticalSection(_critical);
delete _critical;
break;
default:
LeaveCriticalSection(_critical);
}
}
template <class Object>
inline ILink<Object>::ILink(Object* object) :
_count (new unsigned long(1)),
_reference (object),
_critical (new CRITICAL_SECTION)
//
// Create a new ILink from a regular pointer.
// Assign a new counter to the reference and
// set it to one.
//
{
InitializeCriticalSection(_critical);
}
template <class Object>
ILink<Object>::ILink(const ILink<Object>& copy) :
_count (copy._count),
_reference (copy._reference),
_critical (copy._critical)
//
// Create a copy of the ILink. Increase the counter
// by one.
//
{
++(*_count);
}
template <class Object>
inline ILink<Object>::~ILink(void)
//
// On destruction of an ILink, detach it
// from the object.
//
{
_detach();
}
template <class Object>
ILink<Object>& ILink<Object>::operator = (const ILink<Object>& assign)
//
// If the ILink has not been assigned to itself,
// detach it from the previous object. Set the
// reference to point to the assigned object.
// Increase the counter for the assigned object by one.
//
{
if (assign!=*this)
{
_detach();
_count =assign._count;
_reference =assign._reference;
_critical =assign._critical;
++(*_count);
}
return (*this);
}
template <class Object>
inline Object& ILink<Object>::operator * (void)
//
// Dereference the ILink by returning a reference
// of the pointed-to object. If the ILink does not
// point to a valid object throw an exception.
//
{
if (!_reference)
throw std::runtime_error(IDENTIFIER +" : null pointer exception");
return(*_reference);
}
template <class Object>
inline const Object& ILink<Object>::operator * (void)const
//
// Dereference the ILink by returning a const reference
// of the pointed-to object. If the ILink does not
// point to a valid object throw an exception.
//
{
if (!_reference)
throw std::runtime_error(IDENTIFIER +" : null pointer exception");
return(*_reference);
}
template <class Object>
inline bool ILink<Object>::valid(void)const
//
// Return true if ILink is valid.
//
{
return(_reference != 0);
}
template <class Object>
inline bool ILink<Object>::operator ! (void)const
//
// Return true if ILink is invalid.
//
{
return (_reference == 0);
}
template <class Object>
inline bool ILink<Object>::operator == (const ILink<Object>& link)const
//
// Return true if two ILinks are equal
//
{
return (_reference == link._reference);
}
template <class Object>
inline bool ILink<Object>::operator != (const ILink<Object>& link)const
//
// Return true if two ILinks are not equal
//
{
return (_reference != link._reference);
}
A thread safe smart pointer class, or is it not?
Author
Before anyone rips me appart for developing my own smart pointer, I know there's others (boost etc.) out there, this is for the learning experience, thank you ;).
Since I've been experimenting with multi-threading and cross process synchronisation lately I'm trying to get my smart pointer class to be thread safe. I'm using a 'critical section', since a mutex is overkill imho.
I think the class should be thread safe this way, but I'm not sure since I'm fairly new to this. If someone with a little more experience feels like having a look at it, give it a go :) [Other suggestions are also welcome of course].
You mentioned cross process synchronization. You'll need a mutex for that. if you meant synchronization for a single process with multiple threads then your choice of a critical section is correct:)
Cheers
Chris
Cheers
Chris
Author
[random comment]
Yah, I'm using a (named) mutex for the cross process part. As I see it an unnamed mutex could be used here, but as far as I understand it every unnamed mutex will block every other unnamed mutex (in the same process), which is not really what I want here. No need to wait on unrelated threads.
I'm trying to avoid using named mutexes as much as possible due to them needing a unique name. You could probably use a GUID for that, but if cross process usability is needed how do I get two processes to use the same GUID for the same mutex? It is unique after all, LOL.
A critical section should be better performance wise as well. At least from what I read about it.
[/random comment]
In this case I'm more interested in the thread safety, as I don't see my pointer being used across process borders (yet?). Thanks for your answer though =)
To be more specific, is the above example thread safe or not? (Or does it even contain some other grave error)
Yah, I'm using a (named) mutex for the cross process part. As I see it an unnamed mutex could be used here, but as far as I understand it every unnamed mutex will block every other unnamed mutex (in the same process), which is not really what I want here. No need to wait on unrelated threads.
I'm trying to avoid using named mutexes as much as possible due to them needing a unique name. You could probably use a GUID for that, but if cross process usability is needed how do I get two processes to use the same GUID for the same mutex? It is unique after all, LOL.
A critical section should be better performance wise as well. At least from what I read about it.
[/random comment]
In this case I'm more interested in the thread safety, as I don't see my pointer being used across process borders (yet?). Thanks for your answer though =)
To be more specific, is the above example thread safe or not? (Or does it even contain some other grave error)
I didn't take a good look at it, but from what I can tell, this is not even close to being thread safe. The short list:
1) No volatile keyword anywhere.
2) The assignment operator only enters a critical section inside the call to _detach(), so the assignment of _count, _reference and _critical are wide open to threading issues.
3) Copy constructor non-atomic.
And just what are you trying to protect here in the first place? Pointer level thread safety rarely guarantees much of anything. Consider your unary operator * overload: if thread A calls operator *(), there is a context switch to thread B right after it checks for nullness, thread B deletes the pointer and execution switches back to thread A, the operator *() call is going to return a pointer into bad memory. In order for criticial section protection to be useful here, it needs to happen at a much higher level than your pointer class is aware of.
As an aside: your Object * constructor isn't exception safe; it will leak memory on exception in the last two memory allocations. And your self assignment check only checks if the two pointers point to the same object, not if they actually are the same object, which means your comment is incorrect.
1) No volatile keyword anywhere.
2) The assignment operator only enters a critical section inside the call to _detach(), so the assignment of _count, _reference and _critical are wide open to threading issues.
3) Copy constructor non-atomic.
And just what are you trying to protect here in the first place? Pointer level thread safety rarely guarantees much of anything. Consider your unary operator * overload: if thread A calls operator *(), there is a context switch to thread B right after it checks for nullness, thread B deletes the pointer and execution switches back to thread A, the operator *() call is going to return a pointer into bad memory. In order for criticial section protection to be useful here, it needs to happen at a much higher level than your pointer class is aware of.
As an aside: your Object * constructor isn't exception safe; it will leak memory on exception in the last two memory allocations. And your self assignment check only checks if the two pointers point to the same object, not if they actually are the same object, which means your comment is incorrect.
Author
Quote:Original post by SiCrane
I didn't take a good look at it, but from what I can tell, this is not even close to being thread safe. The short list:
1) No volatile keyword anywhere.
2) The assignment operator only enters a critical section inside the call to _detach(), so the assignment of _count, _reference and _critical are wide open to threading issues.
3) Copy constructor non-atomic.
1] Reading up on that right now, thanks.
2] True... if two threads try to assign to the same smart-pointer simultaneously I'll be in trouble, didn't see that, thanks.
3] Yet, more for me to read.
Quote:
1) And just what are you trying to protect here in the first place? Pointer level thread safety rarely guarantees much of anything.
2) Consider your unary operator * overload: if thread A calls operator *(), there is a context switch to thread B right after it checks for nullness, thread B deletes the pointer and execution switches back to thread A, the operator *() call is going to return a pointer into bad memory.
3) In order for criticial section protection to be useful here, it needs to happen at a much higher level than your pointer class is aware of.
1] I'm trying to prevent my smart-pointers from doing a multi-delete. Without the critical section two pointers could decrease count, and then both find count to be zero and decide to delete. With the critical section only one pointer at a time can decrease and check _count. So only one pointer (the last one) should see '_count == 0' and delete.
If the smart-pointers themselves are not safe, what use is it to implement higher level safeguards?
2] This shouldn't be possible as I see it. A delete will only occur once the last smart-pointer to an object goes out of scope. At this point no other thread should still have an instance/copy of this particular smart-pointer.
3] I'm trying to prevent double deletion, and thanks to your suggestion I'll also implement assignment protection. I'm trying to get the smart pointer itself thread safe. I'm doing this by using a 'critical section', I'm not trying to protect a critical (code) section by this.
It is clear to me that this will not automagically render the whole application itself thread safe.
Quote:
1) As an aside: your Object * constructor isn't exception safe; it will leak memory on exception in the last two memory allocations.
2) And your self assignment check only checks if the two [smart-]pointers point to the same object, not if they actually are the same object, which means your comment is incorrect.
1] True. std::nothrow or try/catch...
2] I'm doing "if (assign != *this)", so as I see it I'm checking if the passed instance is 'myself' and not only if 'assign' points to the same 'object' (memory location).
This would be "if (assign._reference == (*this)._reference)", no? (which should not be possible unless assign and this are indeed the same)
I probably shouldn't use the word 'object' to refer to both instances of the class and memory locations :).
I don't understand what you're doing here. Are you (a) experimenting with multi-threading, or (b) solving a real-world problem with this implementation?
* So what you want is a cross-process thread-safe list??? Take a step back and do some reading before you go on. I'd say there are several flaws that makes you don't want to proceed. Rather than letting several processes touch this data structure, you should make a service process that holds this data structure. If other processes needs the data in that list, the should act like clients to that service process to touch whatever data they need. What if one process gets terminated while updating this ultra-global list and holds a lock? You're screwed.
* Makeing thread safe containers is likely not a good idea. The user of the container should do the sync as needed from outside. It's simply not possible to keep locking within the container itself. While it at a first glance might seem like a good idea to abstract the lock handling within the container there are several reasons why this won't work. That it's not done in commonly used libraries, such as STL, MFC, Java class library and .NET Class libraries should be proof enough.
Multithreading isn't trivial. I strongly suggest you read more about multithreading and related problems before going on implementing code. There are plenty of good MSDN articles for legacy Win32 as well as .Net that would be useful for you to read.
Quote:Original post by Wildfire
If the smart-pointers themselves are not safe, what use is it to implement higher level safeguards?
Because the higher level safeguards are going to do the job that your smart pointer is trying to do, but failing at. Proper multithread protection is all about protecting resources and not just protecting the handles to the resources. What your pointer class is doing is giving you a false sense of security without really protecting anything. Furthermore, the additional number of critical sections you're entering and leaving may in fact expose you to more problems since you have a greater chance of deadlocking.
Quote:
2] This shouldn't be possible as I see it. A delete will only occur once the last smart-pointer to an object goes out of scope. At this point no other thread should still have an instance/copy of this particular smart-pointer.
Except that you delete in more cases that just when an object goes out of scope. Consider this situation: Thread A and Thread B both have a reference to an object that contains a smart pointer P that has a _count of 1. A does: (*P).SomeFunction(); B does: P = SomeOtherPointer; Thread schduling has A execute up to right after the if (!_reference) and switches to B. Thread A is now about to return _reference. Thread B assigns a new value to P. P executes the assignment operator, which decrements _count since P is getting a new pointer value. _count hits 0, P deletes _reference. Thread B does other stuff, execution returns to A, A now continues on thinking that the _reference it was about to return is still valid, returns _reference and now is accessing memory that has been deleted.
Quote:
3] I'm trying to prevent double deletion, and thanks to your suggestion I'll also implement assignment protection. I'm trying to get the smart pointer itself thread safe. I'm doing this by using a 'critical section', I'm not trying to protect a critical (code) section by this.
Then there's no point since higher level protection should protect against double deletion as well.
Quote:
2] I'm doing "if (assign != *this)", so as I see it I'm checking if the passed instance is 'myself' and not only if 'assign' points to the same 'object' (memory location).
This would be "if (assign._reference == (*this)._reference)", no? (which should not be possible unless assign and this are indeed the same)
No, since your code has no provision of guaranteeing that two smart pointers to the same object can't be created. If you genuinely believe that it should be the case, then you should check for that situation with an assertion or similar mechanism. This is compounded by the fact that you have a non-explicit Object * constructor, so you can have implicit conversions that will come along and bite you in the rear. Example:
{ int * a = new int; ILink<int> P = a; P = a;}Consider what this code does. It constructs a new ILink around a. Then it tries to assign a to P. However, there is no operator=() overload that takes an int * as a right hand side, but there is an operator=() overload that takes an ILink as a right hand side. So it creates a temporary ILink around a. Then it does assignment, which checks to see if the _references are the same (and they are), so it does a no-op. The temporary is deleted, which deletes a, since the _count was 1. Then P is deleted, which deletes a, since the _count was 1. Double deletion. Boom.
Author
Thanks a lot for your replies SiCrane and Anonymous (Rating++ in case of SiCrane :). I've spent most of the night thinking about it and came to pretty much the same conclusions... there's too many situations that simply are outside of the pointers control. So why try to force it onto the pointer in the first place.
I've found an article on smart pointers and multi-threading when looking for atomic copy constructors that might be interesting to others: Smart Pointers in C++.
Yes, I see it now... it's using the overloaded != operator which will then compare the _reference. Been a long time since I touched that code [rolleyes].
As a general rule a smart-pointer should always be created ILink<int> P(new int(x)); so there's no mixing of regular pointers with smart-pointers. But I can't really enforce this... one could always get a raw pointer like this
'int* a = &(*P)' (or 'int* a=&P.operator *();')
but I'd classify that as asking for trouble [grin]
It says so in my first post: '...this is for the learning experience...I've been experimenting with...'
I'm trying to learn about multi-threading in general, no real world problem behind this so far.
I never said a thing about this being for cross-processing nor did I mention lists. I only said that I was also doing cross-process stuff with mutexes, but not in this case.
In fact I do have a (very simple) cross-process application which shares a file among several processes and uses a mutex so that only one process at a time can read from the file or write to it. But there are no smart pointers involved here.
I never mentioned containers... but like I said above from what you and SiCrane told me and from my own thinking it's a lot clearer to me now that I must protect/control access to the resource. The ressource cannot possibly protect itself against all kinds of pitfalls as it does not possess knowledge about how it is being used/accessed.
I know it's not simple. But I've always been the experimenting type of a learner. Reading a thousand articles never got me so far as one simple applications that crashes on me and I have to figure out why. Most of the times I don't even understand what a particular article is trying to tell me until I run into the same problem myself.
I've found an article on smart pointers and multi-threading when looking for atomic copy constructors that might be interesting to others: Smart Pointers in C++.
Quote:{ int * a = new int; ILink<int> P = a; P = a;}
Consider what this code does. It constructs a new ILink around a. Then it tries to assign a to P. However, there is no operator=() overload that takes an int * as a right hand side, but there is an operator=() overload that takes an ILink as a right hand side. So it creates a temporary ILink around a. Then it does assignment, which checks to see if the _references are the same (and they are), so it does a no-op. The temporary is deleted, which deletes a, since the _count was 1. Then P is deleted, which deletes a, since the _count was 1. Double deletion. Boom.
Yes, I see it now... it's using the overloaded != operator which will then compare the _reference. Been a long time since I touched that code [rolleyes].
As a general rule a smart-pointer should always be created ILink<int> P(new int(x)); so there's no mixing of regular pointers with smart-pointers. But I can't really enforce this... one could always get a raw pointer like this
'int* a = &(*P)' (or 'int* a=&P.operator *();')
but I'd classify that as asking for trouble [grin]
Quote:Anonymous
I don't understand what you're doing here. Are you (a) experimenting with multi-threading, or (b) solving a real-world problem with this implementation?
It says so in my first post: '...this is for the learning experience...I've been experimenting with...'
I'm trying to learn about multi-threading in general, no real world problem behind this so far.
Quote:
* So what you want is a cross-process thread-safe list??? Take a step back and do some reading before you go on. I'd say there are several flaws that makes you don't want to proceed. Rather than letting several processes touch this data structure, you should make a service process that holds this data structure. If other processes needs the data in that list, the should act like clients to that service process to touch whatever data they need. What if one process gets terminated while updating this ultra-global list and holds a lock? You're screwed.
I never said a thing about this being for cross-processing nor did I mention lists. I only said that I was also doing cross-process stuff with mutexes, but not in this case.
In fact I do have a (very simple) cross-process application which shares a file among several processes and uses a mutex so that only one process at a time can read from the file or write to it. But there are no smart pointers involved here.
Quote:
* Makeing thread safe containers is likely not a good idea. The user of the container should do the sync as needed from outside. It's simply not possible to keep locking within the container itself. While it at a first glance might seem like a good idea to abstract the lock handling within the container there are several reasons why this won't work. That it's not done in commonly used libraries, such as STL, MFC, Java class library and .NET Class libraries should be proof enough.
I never mentioned containers... but like I said above from what you and SiCrane told me and from my own thinking it's a lot clearer to me now that I must protect/control access to the resource. The ressource cannot possibly protect itself against all kinds of pitfalls as it does not possess knowledge about how it is being used/accessed.
Quote:
Multithreading isn't trivial. I strongly suggest you read more about multithreading and related problems before going on implementing code. There are plenty of good MSDN articles for legacy Win32 as well as .Net that would be useful for you to read.
I know it's not simple. But I've always been the experimenting type of a learner. Reading a thousand articles never got me so far as one simple applications that crashes on me and I have to figure out why. Most of the times I don't even understand what a particular article is trying to tell me until I run into the same problem myself.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
