RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
319 stars 70 forks source link

Suppress error message at shutdown #719

Closed felixdivo closed 3 years ago

felixdivo commented 3 years ago

Every time I close my node.js server with rclnodejs running I get this error message:

.../my-project/node_modules/rclnodejs/index.js:300
      throw new Error('The module rclnodejs has been shutdown.');
      ^

Error: The module rclnodejs has been shutdown.
    at Object.shutdown (.../my-project/node_modules/rclnodejs/index.js:300:13)
    at process.<anonymous> (.../my-project/node_modules/rclnodejs/index.js:412:7)
    at process.emit (events.js:314:20)

It originates from here: https://github.com/RobotWebTools/rclnodejs/blob/178c2c7068e9c1961f0db3328ab74a366803340e/index.js#L410-L414

I don't quite get why it is raised in the first place, but anyhow, wouldn't it be better if there was a rclnodejs.tryShutdown() method that would be called in that event instead? It would be much like the existing Context.tryShutdown(): Don't do anything if the (default) context is already shut down and else shut it down now.

Is such a change welcome? If yes, I can offer to implement it.

felixdivo commented 3 years ago

This script can reproduce the error:

const rclnodejs = require('rclnodejs');
rclnodejs.regenerateAll().then(() => {
    rclnodejs.init().then(() => {
        const node = rclnodejs.createNode('some_node_name_wow');
        rclnodejs.spin(node);
    })
})
minggangw commented 3 years ago

The reason why you got the error is that you terminated the process before the Promise return by rclnodejs.regenerateAll() is resolved, so the rclnodejs module hasn't been started practically. Normally, generating all the messages triggered by regenerateAll() method takes more around 20s, so you could wait for a while and then press ctrl+c to check if the error is still reported.

It would be much like the existing Context.tryShutdown(): Don't do anything if the (default) context is already shut down and else shut it down now. Is such a change welcome? If yes, I can offer to implement it.

+1, I think it's more user friendly, please go-ahead to submit the PR, thanks!

felixdivo commented 3 years ago

Okay, I'll give it a shot.

felixdivo commented 3 years ago

so you could wait for a while and then press ctrl+c to check if the error is still reported

Seems like it's something else: I waited for an hour (while having lunch) and it still happened.

minggangw commented 3 years ago

Oops, I am going to check it locally.

minggangw commented 3 years ago
const rclnodejs = require('rclnodejs');
rclnodejs.regenerateAll().then(() => {
    rclnodejs.init().then(() => {
        // Please print a log here to ensure the promise has been resolved.
        // I cannot reproduce it on my local linux with develop branch.
        const node = rclnodejs.createNode('some_node_name_wow');
        rclnodejs.spin(node);
    })
})