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

Closes #534 #563

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Closes #534 #563

wants to merge 1 commit into from

Conversation

jreyesr
Copy link

@jreyesr jreyesr commented Dec 22, 2020

Summary

This PR fixes the bug reported in #534: when using a Rosbridge server, there is a race condition that may cause Publisher components (used, for example, in the Publish panel and the -currently unmerged- Teleop panel) to get an undefined rosClient, which causes a panel crash when a message is published. Current workaround is to force a rerender of the Publisher component by changing any of its props (for example, the published topic) as specified in #534 (comment).

This PR fixes the bug by adding a new array of pending publishers. If the setPublishers method of RosbridgePlayer is called before rosClient is set, the publishers are instead buffered on the new array. Any publish() calls are rejected gracefully. When the connect hook is eventually called and rosClient gets a value, the most recent set of pending publishers is sent to setPublishers.

Test plan

This change was tested using the Publish and Teleop panels (which instantiate a Publisher component) and a Rosbridge Websocket server. The test case detailed in #534 (comment) no longer fails.

Versioning impact

This PR is a patch revision.

@jasonimercer
Copy link

@jreyesr , I see foxglove used this commit. We're looking to cherry-pick this into our internal build of webviz. Since you've put up the PR here over a year ago, have you had and negative experience with the change in your builds?

Just trying to exercise extra due diligence as we're looking to incorporate this change late in our internal release schedule.

@jasonimercer
Copy link

I should mention this change has addressed the same issue it was meant to correct (in our tests). We just don't know the webviz architecture enough to understand unintended side effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants