Fix issue with component registration across boundaries in multithrea…

…ded applications
SanderMertens · Mar 21, 2023 · 3fc46cf · 3fc46cf
1 parent 841f8bf
commit 3fc46cf
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 23 deletions.
diff --git a/flecs.c b/flecs.c
@@ -7196,6 +7196,26 @@ const ecs_entity_t* ecs_bulk_init(
  return NULL;
 }
 
+static
+void flecs_check_component(
+ ecs_world_t *world,
+ ecs_entity_t result,
+ const EcsComponent *ptr,
+ ecs_size_t size,
+ ecs_size_t alignment)
+{
+ if (ptr->size != size) {
+ char *path = ecs_get_fullpath(world, result);
+ ecs_abort(ECS_INVALID_COMPONENT_SIZE, path);
+ ecs_os_free(path);
+ }
+ if (ptr->alignment != alignment) {
+ char *path = ecs_get_fullpath(world, result);
+ ecs_abort(ECS_INVALID_COMPONENT_ALIGNMENT, path);
+ ecs_os_free(path);
+ }
+}
+
 ecs_entity_t ecs_component_init(
  ecs_world_t *world,
  const ecs_component_desc_t *desc)
@@ -7204,11 +7224,24 @@ ecs_entity_t ecs_component_init(
  ecs_check(desc != NULL, ECS_INVALID_PARAMETER, NULL);
  ecs_check(desc->_canary == 0, ECS_INVALID_PARAMETER, NULL);
 
+ /* If existing entity is provided, check if it is already registered as a
+ * component and matches the size/alignment. This can prevent having to
+ * suspend readonly mode, and increases the number of scenarios in which
+ * this function can be called in multithreaded mode. */
+ ecs_entity_t result = desc->entity;
+ if (result && ecs_is_alive(world, result)) {
+ const EcsComponent *const_ptr = ecs_get(world, result, EcsComponent);
+ if (const_ptr) {
+ flecs_check_component(world, result, const_ptr,
+ desc->type.size, desc->type.alignment);
+ return result;
+ }
+ }
+
  ecs_suspend_readonly_state_t readonly_state;
  world = flecs_suspend_readonly(world, &readonly_state);
 
  bool new_component = true;
- ecs_entity_t result = desc->entity;
  if (!result) {
  result = ecs_new_low_id(world);
  } else {
@@ -7231,16 +7264,8 @@ ecs_entity_t ecs_component_init(
  }
  }
  } else {
- if (ptr->size != desc->type.size) {
- char *path = ecs_get_fullpath(world, result);
- ecs_abort(ECS_INVALID_COMPONENT_SIZE, path);
- ecs_os_free(path);
- }
- if (ptr->alignment != desc->type.alignment) {
- char *path = ecs_get_fullpath(world, result);
- ecs_abort(ECS_INVALID_COMPONENT_ALIGNMENT, path);
- ecs_os_free(path);
- }
+ flecs_check_component(world, result, ptr,
+ desc->type.size, desc->type.alignment);
  }
 
  ecs_modified(world, result, EcsComponent);

diff --git a/src/entity.c b/src/entity.c
@@ -1820,6 +1820,26 @@ const ecs_entity_t* ecs_bulk_init(
  return NULL;
 }
 
+static
+void flecs_check_component(
+ ecs_world_t *world,
+ ecs_entity_t result,
+ const EcsComponent *ptr,
+ ecs_size_t size,
+ ecs_size_t alignment)
+{
+ if (ptr->size != size) {
+ char *path = ecs_get_fullpath(world, result);
+ ecs_abort(ECS_INVALID_COMPONENT_SIZE, path);
+ ecs_os_free(path);
+ }
+ if (ptr->alignment != alignment) {
+ char *path = ecs_get_fullpath(world, result);
+ ecs_abort(ECS_INVALID_COMPONENT_ALIGNMENT, path);
+ ecs_os_free(path);
+ }
+}
+
 ecs_entity_t ecs_component_init(
  ecs_world_t *world,
  const ecs_component_desc_t *desc)
@@ -1828,11 +1848,24 @@ ecs_entity_t ecs_component_init(
  ecs_check(desc != NULL, ECS_INVALID_PARAMETER, NULL);
  ecs_check(desc->_canary == 0, ECS_INVALID_PARAMETER, NULL);
 
+ /* If existing entity is provided, check if it is already registered as a
+ * component and matches the size/alignment. This can prevent having to
+ * suspend readonly mode, and increases the number of scenarios in which
+ * this function can be called in multithreaded mode. */
+ ecs_entity_t result = desc->entity;
+ if (result && ecs_is_alive(world, result)) {
+ const EcsComponent *const_ptr = ecs_get(world, result, EcsComponent);
+ if (const_ptr) {
+ flecs_check_component(world, result, const_ptr,
+ desc->type.size, desc->type.alignment);
+ return result;
+ }
+ }
+
  ecs_suspend_readonly_state_t readonly_state;
  world = flecs_suspend_readonly(world, &readonly_state);
 
  bool new_component = true;
- ecs_entity_t result = desc->entity;
  if (!result) {
  result = ecs_new_low_id(world);
  } else {
@@ -1855,16 +1888,8 @@ ecs_entity_t ecs_component_init(
  }
  }
  } else {
- if (ptr->size != desc->type.size) {
- char *path = ecs_get_fullpath(world, result);
- ecs_abort(ECS_INVALID_COMPONENT_SIZE, path);
- ecs_os_free(path);
- }
- if (ptr->alignment != desc->type.alignment) {
- char *path = ecs_get_fullpath(world, result);
- ecs_abort(ECS_INVALID_COMPONENT_ALIGNMENT, path);
- ecs_os_free(path);
- }
+ flecs_check_component(world, result, ptr,
+ desc->type.size, desc->type.alignment);
  }
 
  ecs_modified(world, result, EcsComponent);

diff --git a/test/cpp_api/project.json b/test/cpp_api/project.json
@@ -1048,6 +1048,7 @@
  "register_meta_after_reset",
  "reregister_after_reset_w_hooks_and_in_use",
  "reregister_after_reset_w_hooks_and_in_use_implicit",
+ "register_component_w_reset_in_multithreaded",
  "count",
  "count_id",
  "count_pair",

diff --git a/test/cpp_api/src/World.cpp b/test/cpp_api/src/World.cpp
@@ -280,6 +280,28 @@ void World_reregister_after_reset_different_name() {
  ecs.component<Position>("Velocity");
 }
 
+void World_register_component_w_reset_in_multithreaded() {
+ flecs::world ecs;
+
+ ecs.set_threads(2);
+
+ flecs::entity pos = ecs.component<Position>();
+ flecs::entity e = ecs.entity();
+
+ flecs::_::cpp_type<Position>::reset();
+
+ ecs.readonly_begin();
+ e.set<Position>({10, 20});
+ ecs.readonly_end();
+
+ test_assert(e.has<Position>());
+ test_assert(e.has(pos));
+ const Position *p = e.get<Position>();
+ test_assert(p != nullptr);
+ test_int(p->x, 10);
+ test_int(p->y, 20);
+}
+
 template <typename T>
 struct Tmp { int32_t v; };
 struct Test { };

diff --git a/test/cpp_api/src/main.cpp b/test/cpp_api/src/main.cpp
@@ -998,6 +998,7 @@ void World_register_builtin_after_reset(void);
 void World_register_meta_after_reset(void);
 void World_reregister_after_reset_w_hooks_and_in_use(void);
 void World_reregister_after_reset_w_hooks_and_in_use_implicit(void);
+void World_register_component_w_reset_in_multithreaded(void);
 void World_count(void);
 void World_count_id(void);
 void World_count_pair(void);
@@ -5024,6 +5025,10 @@ bake_test_case World_testcases[] = {
  "reregister_after_reset_w_hooks_and_in_use_implicit",
  World_reregister_after_reset_w_hooks_and_in_use_implicit
  },
+ {
+ "register_component_w_reset_in_multithreaded",
+ World_register_component_w_reset_in_multithreaded
+ },
  {
  "count",
  World_count
@@ -5868,7 +5873,7 @@ static bake_test_suite suites[] = {
  "World",
  NULL,
  NULL,
- 93,
+ 94,
  World_testcases
  },
  {