Strings not freed on runtime components

[jpeletier]

Describe the bug

When creating runtime components that contain strings, these strings are not freed on entity destruction, resulting in a memory leak.

To Reproduce

This snippet sets up a hook on the free() function used by Flecs and prints a message if the pointer to the string we are expecting to be deleted is actually deleted. This deletion never happens, thus there is a memory leak.

#include <flecs.h>

ecs_os_api_free_t oldfree;

void *strptr = nullptr; // this will point to the string we are expecting to be freed.

void myfree(void *ptr) {
  if (ptr) {
    if (strptr && strptr == ptr) {
     // print a message if the pointer we are looking for is to be deleted.
      std::cout << "gotcha" << std::endl;
    }
  }
  oldfree(ptr);
}

void main() {

  oldfree = ecs_os_api.free_; // set up a hook on the free() function used by Flecs
  ecs_os_api.free_ = myfree;

  // create a runtime component with one string member
  auto comp = ecs.component("mycomp");
  comp.member(flecs::String, "text");

  flecs::entity e = ecs.entity();
  e.add(comp);

  void *ptr = e.get_mut(comp);

  flecs::cursor cur(ecs, comp, ptr);

  cur.push();
  cur.member("text");
  cur.set_string("hola");
  // get a pointer to the string stored in the component instance:
  uintptr_t *s = (uintptr_t *) cur.get_ptr(); 
  strptr = (void *) (*s);

  cur.pop();

  std::cout << "about to destruct" << std::endl;
  e.destruct(); // at some point here we should expect the string to be destroyed and "gotcha" should be displayed, but it is not.
  std::cout << "after destruct" << std::endl;

}

// output:
// about to destruct
// after destruct

I also could not find in the code (addons/meta) anything that automatically frees up resources related to runtime components.

Expected behavior

When an entity that contains a runtime component is destroyed, all items in that runtime struct should be destroyed.

[copygirl]

edit: I was corrected.

Correct me if I'm wrong but I believe the point of the member() function is to create reflection information for an already existing member on a component (such as a field on a struct). All the function does is create an entity representing the member information and add it to the component entity (see here). What you're doing is registering a tag, a component with no size. It isn't resized to contain any more information, to fit the string pointer.

You can create a component at runtime with a runtime-known size, though I'm not familiar with the C++ API, so I only know of the ecs_component_init function where you provide size and alignment manually. If you're allocating data on components you'll have to implement the correct component hooks to copy and free it appropriately yourself however.

[jpeletier]

@copygirl not quite, you can actually create runtime components, according to this example:

https://github.com/SanderMertens/flecs/blob/37233f127d5006ceb0087fbfcd2f3e36f5b77a23/examples/cpp/reflection/runtime_component/src/main.cpp

This is quite useful for interoperating with scripting, or allowing the user to create components with scripting, which is what I am doing.

With the following excerpt, you will see that the size of this component is 24.

  auto comp = ecs.component("mycomp");
  comp.member(flecs::String, "text"); // 8 bytes
  comp.member(flecs::U32, "some_number"); // 4 bytes
  comp.member(flecs::Bool, "the_bool"); // 4 bytes
  comp.member(flecs::U64, "big_uint"); // 8 bytes

  std::cout << "size=" << comp.get<flecs::Component>()->size << std::endl;

Note that once a component has an entity that uses the component, no further calls to member() are allowed. This makes sense, since there are tables and columns created of a specific size already.

All the function does is create an entity representing the member information and add it to the component entity (see here)

You have to take into account there are hooks/observers watching <Member> being set, for example this one that actually do the job of creating the component.

So yes, it is possible to create runtime components from scratch, as well as add reflection information to existing c++ structs. This is incredibly powerful :-)

My problem is that members of type flecs::String (that store as a pointer to the actual string) are not freed when the component is destroyed.

[copygirl]

Alright, I stand corrected.

My problem is that members of type flecs::String (that store as a pointer to the actual string) are not freed when the component is destroyed.

Does the type guarantee a lifetime for the string that's exactly as long as the component's? Because a string could be allocated with the lifetime of the program, come from static program memory, or be reused for multiple components, in which case automatic freeing would not be what you want.

I'm unsure if it can be used on a runtime component, nor if it's the right choice, but I wonder if an on_remove hook would be the right thing to use.

[jpeletier]

on_remove hook is what I will have to use to fix it on my side without having to touch Flecs.

The reason I think this is a bug is that the API that allows you to write a string via reflection does in fact free the previous string that may be already there. This means that the lifetime management of the string living inside the component instance is not the user's responsibility.

So, in the typical example:

  auto comp = ecs.component("mycomp");
  comp.member(flecs::String, "text");

  flecs::entity e = ecs.entity();
  e.add(comp);

  void *ptr = e.get_mut(comp);
  flecs::cursor cur(ecs, comp, ptr);
  cur.push();
  cur.member("text");
  cur.set_string("hola");
  cur.pop();

When you do cur.set_string("hola");, the "meta" addon first frees the string at the corresponding offset, then allocates a new char* pointer to a copy of the passed string ("hola"). Since the first time you call, everything is set to 0 the first free does nothing. The component instance now contains a pointer to a copy of "hola".

If you then do the following:

  cur.push();
  cur.member("text");
  cur.set_string("adiós");
  cur.pop();

the pointer to "hola" would be freed and then replaced with a new pointer to a copy of "adiós".

The relevant code is here.

Because a string could be allocated with the lifetime of the program, come from static program memory, or be reused for multiple components, in which case automatic freeing would not be what you want.

Using the cursor API you cannot set the pointer directly. When you set a string, it copies your string and keeps ownership of that pointer.

Therefore, the meta addon is managing the memory related to strings, it is just not freeing it once the component instance is destroyed: I was expecting that it would iterate all fields with a cursor and free all non-primitive types and opaques.

[SanderMertens]

Yep, this is a known current limitation of runtime components. What needs to happen is that the meta addon needs to register a generic destructor for freeing runtime components.

This is not very difficult to do, I'll try to take a look at this asap.

[jpeletier]

@SanderMertens ; I wrote the following code, I am sure it is not perfect but the idea here is to iterate the type with a cursor, find all strings and then free them up:

void default_dtor(void *ptr, int32_t count, const ecs_type_info_t *type_info) {
  // The following code walks the ops structure, and sets all values to null, which destroys all strings
  // I ignore "count" for now, I guess I'd have to repeat the below `count` times and advancing `ptr` by the component size each iteration.


  ecs_world_t *world = (ecs_world_t *) type_info->hooks.ctx;
  ecs_meta_cursor_t cursor = ecs_meta_cursor(world, type_info->component, ptr);

  for (ecs_entity_t f = ecs_meta_get_type(&cursor); f;) {
    ecs_meta_scope_t *scope = &cursor.scope[cursor.depth];
    ecs_meta_type_op_t *op = &scope->ops[scope->op_cur];
    if (op->kind == EcsOpPush) {
      ecs_meta_push(&cursor);
      f = ecs_meta_get_type(&cursor);
      continue;
    }
    if (op->kind == EcsOpPop) {
      ecs_meta_pop(&cursor);
      if (cursor.depth != 0)
        ecs_meta_next(&cursor);
      f = ecs_meta_get_type(&cursor);
      continue;
    }
    if (f == flecs::String) {
      ecs_meta_set_null(&cursor); // free the string.
    }
    ecs_meta_next(&cursor);
  }
}

Then, what I am doing is to set this as the destructor for all the runtime types I create:

  if (component.has<RuntimeComponent>()) {
    const flecs::type_hooks_t *h = ecs_get_hooks_id(component.world().c_ptr(), component);
    assert(h)
    flecs::type_hooks_t hooks = *h;
    if (hooks.dtor == nullptr) {
      hooks.ctx = component.world().c_ptr(); // store the world as user context so we can use it in destructor.
      hooks.dtor = default_dtor; // point to destructor function detailed above
      ecs_set_hooks_id(component.world().c_ptr(), component, &hooks);
    }
  }

One thing I am lacking that would be nice for the destructor signature is to have it receive a world pointer, because I am occupying the user context for that, and perhaps other people would need it for their stuff.

Should we also do ecs_meta_set_null for opaque types as well?

It feels a bit hacky, but it works. Is there a better way? We can evolve this conversation until we get to a PR.

Thanks!

[jpeletier]

UPDATE: as part of migrating to v4, I simplified this fix. Here is the updated code:

// Recursively search for strings in the type and free them
void free_strings(ecs_world_t *ecs, void *ptr, const EcsTypeSerializer *ser) {
  // Get a pointer to the first element in the array
  ecs_meta_type_op_t *ops = ecs_vec_first_t(&ser->ops, ecs_meta_type_op_t);

  // get the number of opcodes:
  int32_t op_count = ecs_vec_count(&ser->ops);

  for (int i = 0; i < op_count; i++) {
    ecs_meta_type_op_t &op = ops[i];
    switch (op.kind) {
      case EcsOpOpaque: {
        const EcsOpaque *ct = ecs_get(ecs, op.type, EcsOpaque);
        ecs_assert(ct != NULL, ECS_INTERNAL_ERROR, NULL);
        const EcsTypeSerializer *ser2 = ecs_get(ecs, ct->as_type, EcsTypeSerializer);
        free_strings(ecs, ECS_OFFSET(ptr, op.offset), ser2);
      } break;
      case EcsOpString: {
        char **ppstring = (char **) (ECS_OFFSET(ptr, op.offset));
        ecs_os_free(*ppstring);
        *ppstring = nullptr;
      } break;
      default: {
      } break;
    }
  }
}

void default_dtor(void *ptr, int32_t count, const ecs_type_info_t *type_info) {
  // default destructor for components created at runtime
  // there is a bug whereupon strings in runtime components are not freed:
  // https://github.com/SanderMertens/flecs/issues/1215

  ecs_world_t *world = (ecs_world_t *) type_info->hooks.ctx;
  const EcsTypeSerializer *ser =
      (EcsTypeSerializer *) ecs_get_id(world, type_info->component, ecs_id(EcsTypeSerializer));

  for (int i = 0; i < count; i++) {
    free_strings(world, ECS_OFFSET(ptr, i * type_info->size), ser);
  }
}

[SanderMertens]

Looks promising! Few notes:

• A full solution should also handle vectors, vectors of vectors etc. which requires recursive freeing of resources
• The value of an opaque type will often require custom free logic, e.g. you can't free an std::string or std::vector with ecs_os_free. The only way to handle this correctly is for the opaque type to have a dtor. Also note that as_type doesn't say anything about the actual layout of the type, it's just how the type is "represented" when serialized.

[jpeletier]

Thanks :-) I will take a look at this after your feedback to consider other types. The above is mostly a hack.

Incidentally, we need to add default implementations for at least move assign and copy assign hooks. I just ran into this after debugging a double-free (I believe that by default, when there is no move-assign, components are copied via memcpy, therefore string pointers are copied then freed twice by mistake).

[jpeletier]

Not fixed yet!

[SanderMertens]

Ah, GitHub automatically closed it because the world_t pointer PR got merged!