Unittests pass, and then an invalid memory operation happens after?

[Steven Schveighoffer]

On Wednesday, 27 March 2024 at 21:43:48 UTC, Liam McGillivray 
wrote:
> In my current [game 
> project](https://github.com/LiamM32/Open_Emblem), [something 
> strange](https://github.com/LiamM32/Open_Emblem/issues/20) has 
> happened as of a recent commit. When running `dub test`, all 
> the unittests appear to pass, but then after the last unittest 
> has concluded an "Invalid memory operation" happens. Removing a 
> few lines replaces this error with a segfault, but either way, 
> these errors shouldn't be happening. Weirdly, the commit where 
> these errors first appear did nothing but replace a single 
> class-member function with a seemingly identical function 
> through a mixin template.
>
> The errors seem to be related to object deletion. The traceback 
> output for the first error, and where GDB points to for the 
> second error, is the destructor for my `Unit` class.
>
> You see, every `Unit` object is associated with a particular 
> `Map` object and `Faction` object, which it hold references to. 
> Those objects in turn each have an array of `Unit` objects that 
> they are associated with. In the `Unit` destructor, I have it 
> call the `removeUnit` function in both the associated `Map` and 
> `Faction` objects. The errors are happening in either the 
> `Unit` destructor itself, or the `removeUnit` function that it 
> calls. Until the commit that introduced these errors, the 
> `removeUnit` function was written directly in the `Map` class 
> in it's module, but this commit replaced it with the mixin 
> template `UnitArrayManagement`, which `Faction` also uses.
>
> `Unit` destructor:
> ```
>     ~this() {
>         this.alive = false;
>         if (this.map !is null) this.map.removeUnit(this);
>         if (this.faction !is null) 
> this.faction.removeUnit(this);
>         if (this.currentTile !is null) 
> this.currentTile.occupant = null;
>     }
> ```

The GC does not guarantee destructor order. So this code is not 
valid -- e.g. you can't count on `map` to be a valid object at 
this point.

In my estimation, the code is not correct in principle anyway -- 
if the `map` has a pointer to the `unit`, then neither will be 
collected without both being garbage, and so there is no need to 
do these calls (they are all going away presently).

The *only* thing you should be doing in a destructor is freeing 
non-GC resources.

> I read that the garbage collector *sometimes* but not *always* 
> calls destructors on deletion, which sounds crazy to me.

The GC is not guaranteed to delete memory or run destructors. In 
the current implementation, it will destroy everything at the end 
of the program that was allocated using the GC, but the language 
does not guarantee this.

> The second error, which can be achieved by removing the 
> instances of `writeln` in `removeUnit` (making it seemingly 
> identical now to the version of this function previously 
> defined in the `Map` class) is also strange. It seems to be a 
> case of the `Unit` object calling a `Map` object that no longer 
> exists. However, that's also strange, as the `Map` object is 
> supposed to delete all it's associated units on destruction.

As mentioned, GCs do not work this way -- you do not need to 
worry about cascading removal of anything.

You should assume in the destructor that all references in the 
type that were pointing at GC blocks are now invalid (i.e. 
dangling pointers).

>
> So why are these things even happening *after* the unittests 
> have been run? What else do I need to know about object 
> destruction? What may be happening?

The GC is cleaning up all allocated memory, in *no particular 
order*.

-Steve