diff --git a/lib/action/server.js b/lib/action/server.js index 2c12ec53..0eeab553 100644 --- a/lib/action/server.js +++ b/lib/action/server.js @@ -437,10 +437,8 @@ class ActionServer extends Entity { _executeExpiredGoals(result, count) { for (let i = 0; i < count; i++) { - const goal = result.data[i]; - const goalInfo = new ActionInterfaces.GoalInfo(); - goalInfo.deserialize(goal.refObject); + goalInfo.deserialize(result._refArray[i]); let uuid = ActionUuid.fromBytes(goalInfo.goal_id.uuid).toString(); this._goalHandles.delete(uuid); diff --git a/lib/node.js b/lib/node.js index 16a07541..ee539fff 100644 --- a/lib/node.js +++ b/lib/node.js @@ -425,17 +425,20 @@ class Node extends rclnodejs.ShadowNode { } if (properties.isGoalExpired) { - let GoalInfoArray = ActionInterfaces.GoalInfo.ArrayType; - let message = new GoalInfoArray(actionServer._goalHandles.size); - let count = rclnodejs.actionExpireGoals( - actionServer.handle, - actionServer._goalHandles.size, - message._refArray.buffer - ); - if (count > 0) { - actionServer.processGoalExpired(message, count); + let numGoals = actionServer._goalHandles.size; + if (numGoals > 0) { + let GoalInfoArray = ActionInterfaces.GoalInfo.ArrayType; + let message = new GoalInfoArray(numGoals); + let count = rclnodejs.actionExpireGoals( + actionServer.handle, + numGoals, + message._refArray.buffer + ); + if (count > 0) { + actionServer.processGoalExpired(message, count); + } + GoalInfoArray.freeArray(message); } - GoalInfoArray.freeArray(message); } } diff --git a/test/test-action-server.js b/test/test-action-server.js index 1d69abc2..9bdcbf89 100644 --- a/test/test-action-server.js +++ b/test/test-action-server.js @@ -446,7 +446,7 @@ describe('rclnodejs action server', function () { let result = await handle.getResult(); assert.ok(result); - assert.ok(handle.status, GoalStatus.STATUS_SUCCEEDED); + assert.strictEqual(handle.status, GoalStatus.STATUS_SUCCEEDED); assert.ok(deepEqual(result.sequence, Int32Array.from(testSequence))); server.destroy(); @@ -480,7 +480,7 @@ describe('rclnodejs action server', function () { let result = await handle.getResult(); assert.ok(result); - assert.ok(handle.status, GoalStatus.STATUS_ABORTED); + assert.strictEqual(handle.status, GoalStatus.STATUS_ABORTED); assert.ok(deepEqual(result.sequence, Int32Array.from(testSequence))); server.destroy(); @@ -515,7 +515,7 @@ describe('rclnodejs action server', function () { let result = await handle.getResult(); assert.ok(result); // Goal status should default to aborted - assert.ok(handle.status, GoalStatus.STATUS_ABORTED); + assert.strictEqual(handle.status, GoalStatus.STATUS_ABORTED); assert.ok(deepEqual(result.sequence, Int32Array.from(testSequence))); server.destroy(); @@ -543,7 +543,7 @@ describe('rclnodejs action server', function () { let result = await handle.getResult(); assert.ok(result); // Goal status should default to aborted - assert.ok(handle.status, GoalStatus.STATUS_ABORTED); + assert.strictEqual(handle.status, GoalStatus.STATUS_ABORTED); assert.ok(deepEqual(result.sequence, Int32Array.from([]))); server.destroy(); @@ -567,7 +567,7 @@ describe('rclnodejs action server', function () { await client.sendGoal(goal); await assertUtils.createDelay(500); - assert.ok(server._goalHandles.size, 1); + assert.strictEqual(server._goalHandles.size, 1); server.destroy(); }); @@ -590,10 +590,10 @@ describe('rclnodejs action server', function () { await client.sendGoal(goal); await assertUtils.createDelay(500); - assert.ok(server._goalHandles.size, 1); + assert.strictEqual(server._goalHandles.size, 1); - await assertUtils.createDelay(1000); - assert.ok(server._goalHandles.size, 0); + await assertUtils.createDelay(3000); + assert.strictEqual(server._goalHandles.size, 0); server.destroy(); }); @@ -610,6 +610,24 @@ describe('rclnodejs action server', function () { { resultTimeout: 1 } ); + // Wrap _executeExpiredGoals to capture the goalInfo deserialized from the + // native buffer, so we can verify they match the accepted goals. + const expiredGoalInfos = []; + const origExecuteExpiredGoals = server._executeExpiredGoals.bind(server); + server._executeExpiredGoals = function (result, count) { + const ActionInterfaces = require('../lib/action/interfaces.js'); + const ActionUuid = require('../lib/action/uuid.js'); + for (let i = 0; i < count; i++) { + const goalInfo = new ActionInterfaces.GoalInfo(); + goalInfo.deserialize(result._refArray[i]); + expiredGoalInfos.push({ + uuid: ActionUuid.fromBytes(goalInfo.goal_id.uuid).toString(), + stamp: goalInfo.stamp, + }); + } + origExecuteExpiredGoals(result, count); + }; + let goal = new Fibonacci.Goal(); await client.waitForServer(1000); @@ -620,10 +638,37 @@ describe('rclnodejs action server', function () { ]); await assertUtils.createDelay(500); - assert.ok(server._goalHandles.size, 3); + assert.strictEqual(server._goalHandles.size, 3); + + // Snapshot the accepted goal UUIDs before expiration + const acceptedUuids = Array.from(server._goalHandles.keys()).sort(); + + await assertUtils.createDelay(3000); + assert.strictEqual(server._goalHandles.size, 0); + + // Verify the expired goal info deserialized from the native buffer + assert.strictEqual(expiredGoalInfos.length, 3); + + const expiredUuids = expiredGoalInfos.map((g) => g.uuid).sort(); - await assertUtils.createDelay(1000); - assert.ok(server._goalHandles.size, 0); + // Each UUID must be non-zero (the original bug produced all-zero UUIDs) + for (const info of expiredGoalInfos) { + assert.notStrictEqual(info.uuid, Array(16).fill(0).join(',')); + } + + // All 3 UUIDs must be unique (3 distinct goals) + assert.strictEqual(new Set(expiredUuids).size, 3); + + // The expired UUIDs must match the originally accepted ones + assert.deepStrictEqual(expiredUuids, acceptedUuids); + + // Each expired goal must have a non-zero acceptance stamp + for (const info of expiredGoalInfos) { + assert.ok( + info.stamp.sec > 0 || info.stamp.nanosec > 0, + `Expected non-zero stamp, got sec=${info.stamp.sec} nanosec=${info.stamp.nanosec}` + ); + } server.destroy(); });