Fixes trivially caused memory leaks #282
Draft
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
You can currently create a memory leak by adding a component to an entity and then removing the component or deleting the entity. In most cases, this will leak memory as drop is never called.
This PR fixes that by introducing the changes made to flecs to fix that, and adjusting the dtors so that drop is properly called. Relevant PR can be found here, as well as explanations on why this memory leak is happening: SanderMertens/flecs#1874
This has a breaking change:
ecs_emplace_idis used. This is a known bug: https://discord.com/channels/633826290415435777/939683115109072926/1439766559781294201on_replacehook requires a Default constructor to be set. This is because settingon_replacemeans thatecs_emplace_idcan't be used as flecs internals needs to guarantee a call to the hook if the value is replaced. Theoretically we could get around this limitation if the rust bindings takes more control over the process and handles theon_replacehook call itself.Set behaviour is drastically modified for components with no default constructor:
ecs_emplace_idis used instead ofecs_cpp_set, as the latter requires the use of a ctor, otherwise you can get a drop on uninitialized data leading to UB. This is likely from the assumption that anything that has a dtor, probably also has a ctor in C++. This is not always the case in rust, which means we need special handling. It's possible that this could be fixed on the flecs side of things, but it's not trivial to do so since it's a problem that happens during deferral.Ultimately, this PR introduces dropping on
move_dtor, which is called whenever the destination target is constructed memory and is being memcpy'd into. There could still possibly be bugs or memory leaks I haven't spotted yet, but this should fix the most egregious case and I've added some unit tests to showcase that.