Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix same object being removed multiple times (#2048) #108

Merged
merged 8 commits into from May 22, 2019

Conversation

@gitMarky
Copy link
Contributor

@gitMarky gitMarky commented May 2, 2019

See https://bugs.openclonk.org/view.php?id=2048

gitMarky added 4 commits May 2, 2019
Assume you have an object with action "AbortAction" that has an
AbortCall="RemoveObject", or an abort call that removes the object.

Then, removing the object while it has "AbortAction" will remove it
twice, reducing the Def->Count by 2, while only one object was removed.

This does not do very much, but if you have only one object of that type
and later check for object count after you create a new one, then
creating the new object increases the Def->Count from -1 to 0 and the
new object is not found by Find_ID(), even though it exists.
@@ -101,6 +101,7 @@ class C4Object: public C4PropListNumbered
void Splash();
void RemoveSolidMask(bool fBackupAttachment); // Remove solid mask data, if existing
void MovementDigFreeTargetArea(); // Dig the area free, according to action data
inline void FinishRemoval(bool exit_contents);
Copy link
Member

@walachey walachey May 3, 2019

Why inline? In this case it's not needed to be able to include it (as it's defined in the .cpp file). And compilers will decide themselves anyway what to actually inline and what not.
See https://stackoverflow.com/questions/29796264/is-there-still-a-use-for-inline

Copy link
Contributor Author

@gitMarky gitMarky May 3, 2019

Thanks for the article! So, the purpose was to separate the parts "Start removing the object and do all the callbacks" (=AssignRemoval) and "Actually remove the object" (=FinishRemoval). Making it inline had the intention of telling the compiler to not make it an actual separate function.

Copy link
Contributor

@isilkor isilkor May 3, 2019

Why inline? In this case it's not needed to be able to include it (as it's defined in the .cpp file).
It wouldn't be required even if it was defined here because method definitions inside the class definition always have inline linkage.

AddDbgRec(RCT_DsObj, &rc, sizeof(rc));
}

// ----------------------------------------------------------
Copy link
Member

@walachey walachey May 3, 2019

Do we do this weird formatting with comments in other parts of the codebase, too?

Copy link
Contributor Author

@gitMarky gitMarky May 3, 2019

At least in some parts. Movement code had this to begin with, and I removed some of it. Can remove it again, but for a short time it helped me to better see the structure here.

Anyway, the more relevant question here is: Do we still need the if (status ...) return; statements? I think not, because the callbacks cannot remove the object anymore!

@@ -280,12 +280,19 @@ void C4Object::AssignRemoval(bool exit_contents)

// ----------------------------------------------------------
// Remove particles, no destruction check necessary
ClearParticleLists();
ClearParticleLists(); // TODO: Move this to FinishRemoval(), too?
Copy link
Member

@walachey walachey May 3, 2019

That kind of depends on what the functions are supposed to do, which isn't clear from the names (AssignRemoval vs. FinishRemoval) - and they are also not commented. So for an outsider it is very unclear how to answer this question

Copy link
Contributor Author

@gitMarky gitMarky May 3, 2019

Yes, the TODO was more or less for the reviewer. More on that in the other comment, where FinishRemoval() was introduced.

Copy link
Member

@walachey walachey May 7, 2019

Even the reviewer needs to know what the function is supposed to do - and that should be reflected in the source. Either with a descriptive function name or with a comment. Because if it's ambiguous where something should go, you can be sure that someone puts it into the wrong place (e.g. the next time someone needs a place for a cleanup method and thinks about whether to put it into FinishRemoval or AssignRemoval).
Maybe if the functions have a different premise (e.g. a certain object status) that can be violated if people use it incorrectly, this premise could be reflected in some assertions?

Copy link
Contributor Author

@gitMarky gitMarky May 8, 2019

Yes, the comments were added to the header file in the latest commit. From my point of view, since clearing the particles does not rely on callbacks and does not change the object state, it makes more sense to do it later when the object is actually removed.

The more pressing issue here is, that the PR implementation breaks scenario sections, see the comment after my additional commits. I am unsure how to address this.

At first, the fix seemed pretty simple, but it is not :)

// Set status to deleting, so that callbacks in this function that might delete
// the object do not delete it a second time.
// TODO: This causes problems if the status should change during one of those
// callbacks, or if the status was C4OS_INACTIVE
Copy link
Contributor Author

@gitMarky gitMarky May 4, 2019

See the additional TODO:
Either a) we keep the old "if (Status == C4OS_DELETED) return" calls to be safe and remove the C4OS_DELETING again; or
b) we make "object is being deleted" a bool that is independent from the status?

I guess the following options are not a valid solution:
c) Make the status bitwise, instead of exclusive. So you could have Status = C4OS_DELETING | C4OS_INACTIVE. This is not really safe, because I assume that status was assigned directly in other places, meaning that we have to adjust other files, too
d) Set the status C4OS_DELETED at the beginning of the function and remove the C4OS_DELETING again. This was probably not done, because the rest of the engine assumes that the object is gone when the status is C4OS_DELETED, so the callbacks might not do their stuff properly if the status is set beforehand?

@gitMarky gitMarky requested review from isilkor and walachey May 6, 2019
gitMarky added 2 commits May 17, 2019
This solves the problem that adding a new status introduced.
These were necessary before, because the object could be deleted in each
callback. Now we make sure that the function runs without the object
being deleted a second time, so the callbacks could be removed.

With the less bloated code the recently introduced function
FinishRemoval became unnecessary.
@gitMarky gitMarky merged commit d35334a into openclonk:master May 22, 2019
2 checks passed
@gitMarky gitMarky deleted the mantis_2048 branch Jun 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants