Jump to content
  • Advertisement
Sign in to follow this  
shurcool

Does anyone do pointer = null, delete past value of pointer?

This topic is 3539 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hi, I've noticed recently I do a few of:
delete pPointer; pPointer = nullptr;
However, that might be dangerous if pPointer is used from multiple threads. Say, for example, pPointer is actually a pointer to some engine sub-system that is accessibly from multiple threads. It can be the logger class, for instance. Now, if you delete it first, and then assign nullptr (which is defined as 0) to it, another thread might end up using a semi-deleted object. So I wrote a function:
template <typename T>
void SetNullptrAndDelete(T *& pPointer)
{
	T * pPointerCopy = pPointer;
	pPointer = nullptr;
	delete pPointerCopy;
}
I'm just curious, does anyone else have to do something like that? Or is this indicative of a problem in design that can be avoided? Right now the pointers I delete are in fact globals, which I know isn't good. I'm in the process of refactoring the whole thing, and one of the goals is not to have any globals left at the end. But even then, I will have some sub-systems that are accessible from multiple threads, so it seems SetNullptrAndDelete will be needed anyway. I just wanna ask if what I'm doing is sane and maybe even common? Or am I doing something really bad here? Thanks!

Share this post


Link to post
Share on other sites
Advertisement
No this is not thread safe at all. You should not delete a resource until all the threads are done using it. A wrapper class that implements reference counting might work well.

The reason this will not work is not that your function is wrong and it would safely set the pointer to nullptr (might help too if it was volatile) but that the use of the pointer would not be thread safe. Even if you are checking if the pointer is nullptr every time before you use it.

For example say Thread1 has just checked if the pointer is valid and now uses it to call a function. Thread2 decides to delete the pointer. Thread1 could be somewhere in the middle of that function call when thread2 makes the call to delete. In that case for thread1 its this pointer is not pointing to valid memory half way through a function call.

Share this post


Link to post
Share on other sites
Why do you need to delete the same object from different threads? The object should only be deleted once and only one thread or other object should be the owner and responsible for deleting.

Your function does not actually help with the problem since the operation it performs is not atomic. Another thread still can read the pointer value before you are able to set it to 0. The thread that is about to delete the object then can get CPU time and actually delete the object and when the other thread gets its time slice it has a pointer to the deleted object.

It won't work this way, you need thread safe stuff (semaphores, mutexes, locks etc.) for this to work which adds runtime overhead and programming overhead.

Just delete the object only from a single spot when it is sure that it will no longer be required/accessed by anything else.

Share this post


Link to post
Share on other sites
Quote:
Original post by shurcool

So I wrote a function:
template <typename T>
void SetNullptrAndDelete(T *& pPointer)
{
T * pPointerCopy = pPointer;
pPointer = nullptr;
delete pPointerCopy;
}


That doesn't do anything, especially not what think it does.

Quote:
But even then, I will have some sub-systems that are accessible from multiple threads, so it seems SetNullptrAndDelete will be needed anyway.


Nope. The way such a problem is approached is to manage all systems in main thread:
createAndInitializeSystems();

createThreads();
while (running) {
// do the stuff
}
waitForThreadShutdown();

destroySystems()


Quote:
Or am I doing something really bad here?


What you're doing is completely and entirely equivalent to:
delete pPointer;
Considering that deletion and assignment are nor transacted nor atomic, nulling the pointer is unreliable at best.

Share this post


Link to post
Share on other sites
You guys have given me something to think about. Thanks.

Let me look into what exactly I was doing/using this for yesterday, and I'll get back here. :)

Share this post


Link to post
Share on other sites
Ok, I think I know what happened here.

Let me add that I do in fact all my 'delete pSubSystems;' in the main thread, and I do it after/during all the threads are being deleted. So no threads will be deleting these objects, although they might use them when alive.

So, in my main thread's Deinit() function, I had a bunch of:
delete pLogger; pLogger = nullptr; // example only
delete pSystem1; pSystem1 = nullptr;
delete pSystem2; pSystem2 = nullptr;
// etc.


The reason I had to set the pointers to null after deleting them was not to prevent a race-condition or to make the deletion thread-safe (like you guys have eloquently put it, it won't work for that purpose). It was done for a simple reason: The destructor for pSystem2, for example, might check if pLogger is alive (i.e. non-null) and if so, it will use it.

So during normal shutdown of my app, since the logger is deleted first (this is actually a made up example, but it's easier to explain), pSystem2's destructor will see pLogger == nullptr and won't use it.

But if pSystem2 is deleted (from the main thread, of course) earlier on during the execution, at that time pLogger is indeed non-null and will get used.

So for that purpose and that purpose only, setting pPointers to null after deleting them from main thread is completely thread-safe.

Here's where the problem happened: When looking at those delete p; p=null; statements many weeks later, on a late night, I thought for some reason I needed to make those deletes 'more' thread-safe and figured first setting p to null and then deleting was the proper approach.

Yeah, so that's what happens when you're looking at old code without thinking much.

Anyway, I hope that explains it and that what I'm doing NOW is actually fine. So for my original purpose, I don't need to set pointer to null first. I can do it after deleting, since there will only be only the main thread left at that time.

Share this post


Link to post
Share on other sites
Quote:
template <typename T>
void SetNullptrAndDelete(T *& pPointer)
{
T * pPointerCopy = pPointer;
pPointer = nullptr;
delete pPointerCopy;
}

Actually there can be some point in copying the pointer, nulling the original, then deleting on the copy. MS ATL code for dealing with COM objects does exactly that for a reason, it solves obscure re-entrancy problems...

Lets say delete is called from the Release method of a smart pointer and it doesn't do the above. Release is responsible for dropping the refcount to zero in this case and hence decides the object should now be deleted.
Now, imagine a scenario whereby the object being deleted owns other sub-objects (which will also be destroyed by this delete) and that those sub-objects have raw pointers to its parent (as you do to break reference cycles). Now, what if during the destructor of those sub-objects initiated by the destruction of their parent, those sub-objects happen to increase, and then decrease the ref-count of its parent? (Hey, it could happen). What will happen is that Release gets called again with the refcount going from 1 to zero, and the object gets deleted again. Hence something like the above is done.

Share this post


Link to post
Share on other sites
Quote:
Original post by shurcool
setting pPointers to null after deleting them from main thread is completely thread-safe.

It's not. First you need some kind of mutex to make sure your object isn't being used by another thread (in which case setting the pointer to null is pointless because it's value has been read already) Second, even when your object isn't being used, you need to consider compiler and CPU reordering and cached memory.
All of which sounds to me like trouble and bad design for this particular problem.

A better design: before you shut down something, shut down everything that depends on it.

In your situation that probably means signaling that second thread it needs to stop (using some kind of message), waiting until you receive confirmation of shutdown (like WaitForSingleObject on the thread's handle), then delete you pointer.

Share this post


Link to post
Share on other sites
Quote:
Original post by janta
Quote:
Original post by shurcool
setting pPointers to null after deleting them from main thread is completely thread-safe.

It's not. First you need some kind of mutex to make sure your object isn't being used by another thread.

You kinda took my statement out of context, and I probably didn't word it in the best way.

Quote:
Let me add that I do in fact all my 'delete pSubSystems;' in the main thread, and I do it after/during all the threads are being deleted. So no threads will be deleting these objects, although they might use them when alive.

<snip>

So for that purpose and that purpose only, setting pPointers to null after deleting them from main thread is completely thread-safe.

So when I said thread-safe at the end, I meant that it had nothing to do with thread-safety since it is trivially ensured: no threads will exist at that time.

Quote:
A better design: before you shut down something, shut down everything that depends on it.

And that's exactly what I'm doing. :)

It works well, although what would you do about circular dependencies? (So far, my only solution is to avoid them)

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!