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

Port #464, #478, #496, and #502 from ros1 branch #663

Conversation

DomenicP
Copy link
Contributor

Public API Changes

  • Synchronize SubscriberManager methods with a threading.Lock
  • Add block parameter to MessageHandler.finish() method with default value of True
    • Use this parameter to allow QueueMessageHandler.finish() to run without blocking
  • Add IncomingQueue class to websocket_handler.py to decouple incoming messages from the IOLoop thread

Description

Earlier this year I ran into some of the issues described in #425 where the rosbridge server would end up deadlocked or blocked. I was able to solve the problem by porting several PRs from the ros1 branch (which was develop at the time) to my working branch.

I recently rebased my working branch to a newer upstream version and after testing confirmed that these fixes are still needed for my application. I thought I would open a PR and see if there was any interest in getting these features into the main ROS2 branch. All credit to the original PR authors; I simply ported the changes over from the ROS1 branch to ROS2.

@DomenicP DomenicP force-pushed the port-deadlock-and-blocking-bugfixes branch 2 times, most recently from 17a114b to 74450f6 Compare October 1, 2021 16:19
@DomenicP DomenicP force-pushed the port-deadlock-and-blocking-bugfixes branch from 74450f6 to 172c7fb Compare October 1, 2021 16:22
@DomenicP DomenicP marked this pull request as ready for review October 5, 2021 00:13
@amacneil amacneil merged commit cc5209c into RobotWebTools:ros2 Oct 7, 2021
@amacneil
Copy link
Member

amacneil commented Oct 7, 2021

Thanks!

@DomenicP DomenicP deleted the port-deadlock-and-blocking-bugfixes branch October 20, 2021 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants