Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enforce correct message type is passed to various API #317

Merged
merged 2 commits into from
Apr 24, 2019

Conversation

dirk-thomas
Copy link
Member

Closes #314.

The PR contains two commits:

  • Adding tests passing an invalid type to the various APIs PR and expecting a TypeError to be raised. CI with these tests failing: Build Status ( intentionally failing tests)
  • Update the implementation to check the passes type and raise TypeError in case it doesn't match the expected type: Build Status

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas added enhancement New feature or request in review Waiting for review (Kanban column) labels Apr 22, 2019
@dirk-thomas dirk-thomas self-assigned this Apr 22, 2019
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member Author

Full build to ensure no code relies on using the API in that way: Build Status

@dirk-thomas dirk-thomas changed the title add tests ensuring TypeError is raised enforce correct message type is passed to various API Apr 23, 2019
@sloretz sloretz self-requested a review April 23, 2019 20:22
future = cli.call_async(GetParameters.Request())
executor = rclpy.executors.SingleThreadedExecutor(context=self.context)
with self.assertRaises(TypeError):
rclpy.spin_until_future_complete(self.node, future, executor=executor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this traceback look like? I would have expected the exception to be swallowed here:

rclpy/rclpy/rclpy/task.py

Lines 212 to 213 in afc4cd4

except Exception as e:
self.set_exception(e)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Output of the test without the with self.assertRaises(TypeError)::

rclpy/__init__.py:204: in spin_until_future_complete
    executor.spin_until_future_complete(future, timeout_sec)
rclpy/executors.py:271: in spin_until_future_complete
    self.spin_once(timeout_sec=timeout_sec)
rclpy/executors.py:628: in spin_once
    raise handler.exception()
rclpy/task.py:206: in __call__
    self._handler.send(None)
rclpy/executors.py:382: in handler
    await call_coroutine(entity, arg)
rclpy/executors.py:337: in _execute_service
    srv.send_response(response, header)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <rclpy.service.Service object at 0x7f549dd3bc88>
response = 'different response type'
header = <capsule object "rmw_request_id_t" at 0x7f549dd3c4e0>

    def send_response(self, response: SrvTypeResponse, header) -> None:
        """
        Send a service response.
    
        :param response: The service response.
        :param header: Capsule pointing to the service header from the original request.
        :raises: TypeError if the type of the passed response isn't an instance
          of the Response type of the provided service when the service was
          constructed.
        """
        if not isinstance(response, self.srv_type.Response):
>           raise TypeError()
E           TypeError

rclpy/service.py:79: TypeError

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh I didn't know about

if handler.exception() is not None:
raise handler.exception()
That's a nice improvement

@dirk-thomas dirk-thomas merged commit 4bf61f4 into master Apr 24, 2019
@delete-merged-branch delete-merged-branch bot deleted the raise_type_error branch April 24, 2019 00:18
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publishing a message of the wrong type can cause a hard error
2 participants