Sign in to follow this  
VBStrider

Code Design: Object deleted before higher code is completely finished with it

Recommended Posts

I've been running into this design problem for a while, and it has been bugging me. I am posting it here in the hope that it will finally be resolved.
CClass *objects[MAX_OBJECTS];

void Foo()
{
	for (int i = 0; i < MAX_OBJECTS; i++)
	{
		if (objects[i] != NULL)
		{
			Bar(i);
			objects[i]->A();
		}
	}
}

At some point in Bar(), the object #i may be deleted. If this happens, everything after Bar() that tries to access object #i will fail. I know multiple ways that this can be solved. However, because I am relatively new to code design and OOP, I don't know if there is a standard/common/best solution. I could choose one of my solutions at random, but I would rather learn what the "proper" solution is, if such a thing exists. I have not posted my own solutions here because I don't want to accidentally influence responses. If requested, I can provide my thoughts on the matter. Please note that I am looking for an abstract solution. If the solution for this problem is dependent on the exact situation, please let me know and I will give a specific situation in which this problem is occurring.

Share this post


Link to post
Share on other sites
Thank you for the quick reply.

Please don't consider this topic "solved" just yet though. I am hoping for multiple responses, and possibly a discussion of the issue.

Share this post


Link to post
Share on other sites
The general-purpose design pattern is a deferred processing queue.

Maintain a list of callback functions to be executed when everything else is done. In cases like this, where you have work to be done at some later time, add it to the list. A good time to run the list is at the end of every frame in your main loop.



A "delete me later" list is a special case of the general pattern. It may be the easier solution if you are only doing the one case.

Share this post


Link to post
Share on other sites
Quote:
Original post by frob
A "delete me later" list is a special case of the general pattern. It may be the easier solution if you are only doing the one case.
The issue is that even if you have the general deferred processing queue, you may *still* need the 'delete me later' list, because some deferred callbacks may depend on the item to be deleted (and it isn't trivial in the general case to figure out which).

Share this post


Link to post
Share on other sites
Quote:
Original post by VBStrider
At some point in Bar(), the object #i may be deleted. If this happens, everything after Bar() that tries to access object #i will fail.

No, it won't fail. It will do something undefined, and if state isn't modified too much, will even succeed.

Invalid memory access in C++ is merely undefined, and never an error.

First rule: Whoever allocates, also de-allocates. This solves the problem completely. Bar() isn't allowed to de-allocate.

For everything else, either use managed references or value types.

Also note that your example doesn't show who allocates.


Quote:
but I would rather learn what the "proper" solution is

"Proper"?

void Foo(vector<CClass> & entities);
void Baz(CClass & entity);

Neither of them deals with resource management in any way.

Quote:
may be deleted
What exactly does 'deleted' mean? That delete is called on an instance? That specific instance is no longer needed? That it must be shredded right here and right now?

Or - does it mean that that particular instance is no longer part of objects?

If last, then the problem has to deal with single responsibility. Baz becomes redundant then, and Foo needs to take care of both, since only Foo knows about container, Baz only knows about individual instance.


Quote:
Please note that I am looking for an abstract solution.
Before looking for a solution, first define the actual result you wish to accomplish. Not a for loop, or array, but "I have a list of all loaded sprites which are automatically loaded when needed" or similar.

Share this post


Link to post
Share on other sites
"Deleted" means that the object is invalid, no longer usable, doesn't exist, etc. The exact meaning depends on the solution used (delayed deletion, immediate delete and later checks, etc).

Quote:
It will do something undefined, and if state isn't modified too much, will even succeed. Invalid memory access in C++ is merely undefined, and never an error.

Which is a failure... When I said "fail", I did not mean to attach any language or platform specifics to it. The word is used to mean "does not work correctly".

Quote:
Before looking for a solution, first define the actual result you wish to accomplish. Not a for loop, or array, but "I have a list of all loaded sprites which are automatically loaded when needed" or similar.

One example of where this becomes a problem is in network packet handling. For this example, objects are called clients and Bar() handles network packets for client #i. Among other reasons, an error in a packet could cause the client to be forcibly disconnected (and thus, the client object would be removed). The call to CClass::A() represents any code that would be executed after the client has been removed.

Another example is where Foo() is managing the characters (and possibly other things) in the game. The object is a character. Bar() represents one of possibly multiple character management functions where the character could become removed from the world due to death or other reasons specific to the game. The call to CClass::A() represents any code that would be executed after the character has been removed.

Share this post


Link to post
Share on other sites
Why are you storing pointers to the objects rather than storing objects directly?

What does it *mean* for the object to be deleted? If 'Bar' concludes that the object should be deleted, do you still want to call A() on it before deleting it?

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
Why are you storing pointers to the objects rather than storing objects directly?

It is the way I decided to write the example code. I could have easily used an array of objects instead, and the problem would still hold.

Quote:
Original post by Zahlman
What does it *mean* for the object to be deleted? If 'Bar' concludes that the object should be deleted, do you still want to call A() on it before deleting it?

No, A() should not be called on the object after the object has been deleted. When an object is deleted, it is no longer usable.

Share this post


Link to post
Share on other sites
I'm having trouble with this thread simply because this isn't a situation I could see myself getting into, at least not like the example code.

If you call a function which can cause something to go into an invalid state then common sense says you should check that it is still valid in some way. The way depends on the rest of the system; if could be as simple as the function returning a value to indicate the object is dead, to an object which is simply no longer in a valid state but still 'active' enough that it can be called on (just wont do anything).

Even your examples aren't great;
- if the outcome of 'bar' is that client #2 should be disconnected and removed from the game then this would be flagged and carried out at a latter clean up step (beause its unlike to be as simple as 'delete client[2]'). If you include that into 'bar' then 'bar' starts having to care about more things than it should (which would probably end up causing the function name to be a lie)

- Same thing applies to the 'character in a world' setup. You would perform your world update and then check to see who needs to be removed so that on your next sweep you simply no longer have it there to worry about. If something dies on frame 24579 of a game then it should be sane to keep the entity around until the end of that frame, or indeed a 'validity' sweep at the start of frame 24580 so that references can be taken care of and things generally cleaned up.

In short 'bar' shouldn't be deleting anything, 'bar' should be putting things into a state where someone else can initaliate the clean up.

Share this post


Link to post
Share on other sites
Quote:
Original post by phantom
I'm having trouble with this thread simply because this isn't a situation I could see myself getting into, at least not like the example code.

Even your examples aren't great;

That was the intent of the examples, considering we are discussing a design issue here. I specifically avoided giving solutions since the purpose of this thread is to see how other people are solving the problem.

Thank you for your input. I am currently looking into a system similar to what you describe. If anyone has more to add though, please reply.

Share this post


Link to post
Share on other sites
Depending on the situation, you may also consider throwing and catching exceptions. Probably not in the character case but for maybe in the client case (although, I suppose a disconnect could be considered normal in which case exceptions are ugly). For example if you had a list of open files and 'Bar()' noticed that 'i' was no longer readable for an unexpected reason (user pulling a usb stick or something) you could use exceptions and remove it from the list in the catch-block.

Share this post


Link to post
Share on other sites
I think you may want to consider handling object lifetimes differently. Generally with object lifetimes there's two models I use, which greatly simplify things:

1) Ownership is the first model. Ownership is very simple. Object A owns object B. Perhaps, container C owns object B. Whatever it may be, the lifetime of B is entirely contained within another object. Only that object is responsible for deleting the object. In this case, it may simply be a member and go away when the object does, may be directly owned as an element in the container (non-pointer), or may be held in a non-reference counted smart pointer (e.g.: scoped_ptr<>). This is the simplest model because once the object has an owner, it's extremely easy to determine the relationship. If the object is no longer used, you remove it and forego any further processing on this object.

2) The second possibility is shared ownership, typically using a reference-counting smart pointer, such as shared_ptr<>. The benefit here is you don't need to worry about ownership. When the object isn't being referenced anymore, it will be deleted. It simplifies your concerns. The downside is it's harder to know when it will actually get deleted, since the ownership concept is not so clear.

Your example violates the first principle, because deleting the object without notifying the array has abused the ownership principle. You can't delete the object and have the array pointing at garbage. That's an inconsistent state in your program.

Your example also violates the second principle, because you said it has the potential of being deleted while it is still being used, as it is still part of the array.

I would try to rectify your design to use one of the two mechanisms. In your case, I would probably advise an ownership mechanism such as this:


std::vector<CClass> objects(MAX_OBJECTS);

void foo()
{
for (std::vector<CClass>::const_iterator iter = objects.begin(); iter != objects.end(); )
{
// Return false, for example, if the object should be removed
if (!Bar(*iter))
{
iter = objects.erase(iter);
}
else
{
iter->A();
++iter;
} // end if
} // end for
} // end foo


Share this post


Link to post
Share on other sites
Quote:
Original post by VBStrider
One example of where this becomes a problem is in network packet handling. For this example, objects are called clients and Bar() handles network packets for client #i. Among other reasons, an error in a packet could cause the client to be forcibly disconnected (and thus, the client object would be removed). The call to CClass::A() represents any code that would be executed after the client has been removed.

This is an interesting example because I have to contend with exactly this problem in some code I work with.

The proper solution - and yes, I call it proper because I strongly believe any other way is wrong - is that the packet parsing code should never be able to perform an operation that destroys the client object. That code should know nothing about clients or networks and shouldn't have any interest in the lifetime of those objects. It should have one single responsibility, which is to understand the packet. If the packet was valid, return the parsed content or a success value or both. If the packet was invalid, return an error code or raise an exception. The code above that, ie. the code that called it, will decide how to handle that situation, and that code should be the code that actually owns the client object and has authority over deleting it. After all, what if you wanted to have it so that a corrupted message is resent, or a diagnostic message is sent, or some other behaviour? This shouldn't be done by the code that finds the error, but by the code that owns the object. Give it the error, and let it decide what to do.

In your example, it should be clear that Foo() needs implicit ownership of the objects array, in order to iterate over it safely. Therefore Foo() also has ownership of the contents of the array. Bar(), therefore, has no ownership of those objects and should not be deleting anything at all. When you consider that Foo is a high level part of the app that owns Clients and Bar is just a low level packet parsing function, this makes intuitive sense.

Really, you don't want these low level functions having sporadic side-effects where sometimes they change your data and sometimes they don't. Ideally, either a function should always be performing a predictable sort of mutation on an object or it should be doing nothing at all. Keeping to this rule makes your code a lot easier to debug because you're not always having to guess at the results of each operation.

You must know who 'owns' the object at all times, and when you 'lend' that object to other functions, those functions must never be allowed to destroy the object. Instead, they must signal a wish to have it destroyed when they relinquish control.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this