RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/jazzy/Concepts/Basic/About-Client-Libraries.html#community-maintained
Apache License 2.0
332 stars 72 forks source link

Improve resiliency of entities when destroyed #897

Closed wayneparrott closed 1 year ago

wayneparrott commented 1 year ago

Issue #896 identifies an race condition where multiple subscriptions listen to a common topic and one subscription, from it's callback destroys another subscription (an entity). This revealed an issue where i/o activity can exists on an entity's _handle after the entity has been destroyed. The result is an invalid entity pointer or worse a SEGFAULT. To address this I've added destroy state semantics to the Entity class.

full disclosure: One quirky behavior I've attempted to resolve is that setting Entity._handle = null, results in the _handle being quickly GC'ed. This causes the SEGFAULT when the handle is still referenced by ShadowNode.Execute(). I believe after calling _handle.release(), the handle must not be GC'ed until after the current Node.execute() completes. I was not sure how to pull this off in C with Nan API so I implemented a hack by using a cache to hold _handle's after their release(). The cache will flush when its size exceeds a const (100 atm). I'm definitely open to revising this hack with the suggestion of others.

entity.js

entity.d.ts

node.js

action/client.js

action/server.js

main.ts

test-destruction.js

Fix #896

wayneparrott commented 1 year ago

@minggangw my latest commit 9af468f to address your suggestion may have overwritten f3840e2

minggangw commented 1 year ago

@minggangw my latest commit 9af468f to address your suggestion may have overwritten f3840e2

Sorry for missing this comment, the change looks good to me, thanks!

minggangw commented 1 year ago

Hi @wayneparrott I have approved this PR, please go ahead to merge it after resolving the conflict, thanks!