Avoid reference cycle between Node and ParameterService #490
Conversation
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno
added
enhancement
|
Why did you choose to refactor all methods to local function? This seems to make it much harder to write tests for the numerous functions since they are unaccessible from the outside. Wouldn't a |
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Actually, it wasn't just only bad for unit testing, it wasn't breaking the reference cycle (1). I now tested this together with #488, and #470 destruction order is ok. |
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
rclpy/rclpy/parameter_service.py
Outdated
| def _get_node(self): | ||
| node = self._node_weak_ref() | ||
| if node is None: | ||
| raise RuntimeError('Expected valid node weak reference') |
There was a problem hiding this comment.
Should this be a ValueError instead (or what is the reason to choose RuntimeError)?
There was a problem hiding this comment.
AFAIU, ValueError is used when an argument passed by the user is not of the appropiate value. There's not "argument" involved here, just an attribute of the object being invalid.
RuntimeError is the choice when the error doesn't fall in any of the other categories (link), and I don't think there is a more appropiate standard error.
I also can create a custom error if that sounds better.
There was a problem hiding this comment.
I was mostly wondering because RuntimeError is treated differently by the command line tools - they suppress the stacktrace and only show the exception string. Not sure if that is intended in this case.
There was a problem hiding this comment.
It sounds like ReferenceError is what we want here.
There was a problem hiding this comment.
It sounds like ReferenceError is what we want here.
Sounds like the right option! Updated the PR.
I was mostly wondering because RuntimeError is treated differently by the command line tools - they suppress the stacktrace and only show the exception string. Not sure if that is intended in this case.
A little off-topic:
I have observed this, and find that behavior strange. Why do the command line tools do that?
There was a problem hiding this comment.
Some kind of error messages don't warrant to show a stacktrace to the user. And in those cases we use RuntimeError.
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
|
CI up to rclpy, only fastrtps:
|
delete-merged-branch
bot
deleted the
ivanpauno/avoid-node-parameter-service-refcycle
branch
See #470 (comment).