Skip to content

Fix race condition in websocket test (backport #1185)#1187

Merged
bjsowa merged 2 commits intojazzyfrom
mergify/bp/jazzy/pr-1185
Mar 17, 2026
Merged

Fix race condition in websocket test (backport #1185)#1187
bjsowa merged 2 commits intojazzyfrom
mergify/bp/jazzy/pr-1185

Conversation

@mergify
Copy link
Copy Markdown

@mergify mergify bot commented Mar 17, 2026

Public API Changes
None

Description
This fixes an issue with the websocket testing instrumentation which had a race condition inherent in it that was causing my test to sometimes fail incorrectly (false negative) which led me to believe that other solutions were helping with other issues (they were not).

Changes:

  1. Fix websocket testing instrumentation race condition when many clients requested at once. You can verify this fix by trying this before and after the fix:
colcon test --packages-select rosbridge_server --ctest-args -R "test_startup_race" --repeat until-fail:500 --event-handlers console_direct+ 2>&1 | tee /tmp/startup_race_retry.txt
# in another terminal:
stress --cpu 15

Eventually this should fail, especially with stress running at the same time (load your CPU as much as possible). The fix was to use the reactorThread for sending messages and to add retries to getting the ros port parameter from the ros node.
3. Removes the startup_race.test.py test which, it turns out, was mostly exercising the websocket test instrumentation and not any particularly useful code paths.
4. Reverts the previous "fix" I made to the server instantiation order. Upon further examination, I realized that the order does not matter since the websocket requests are not processed in rosbridge_websocket.py until we call await stop_event.wait(), which is already after the ros2 executor and node are all started. I can remove this change if we want to keep the order of instantiation I had changed to earlier (either way should work fine).


This is an automatic backport of pull request #1185 done by [Mergify](https://mergify.com).

* Integration test instrumentation fix

* Revert unnecessary startup fix and remove corresponding always passing test

(cherry picked from commit 85ff1cd)

# Conflicts:
#	rosbridge_server/CMakeLists.txt
#	rosbridge_server/test/websocket/startup_race.test.py
@mergify mergify bot added the conflicts label Mar 17, 2026
@mergify
Copy link
Copy Markdown
Author

mergify bot commented Mar 17, 2026

Cherry-pick of 85ff1cd has failed:

On branch mergify/bp/jazzy/pr-1185
Your branch is up to date with 'origin/jazzy'.

You are currently cherry-picking commit 85ff1cd.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   rosbridge_server/scripts/rosbridge_websocket.py
	modified:   rosbridge_server/test/websocket/common.py

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   rosbridge_server/CMakeLists.txt
	deleted by them: rosbridge_server/test/websocket/startup_race.test.py

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@bjsowa bjsowa removed the conflicts label Mar 17, 2026
@bjsowa bjsowa merged commit 34d67d5 into jazzy Mar 17, 2026
5 checks passed
@bjsowa bjsowa deleted the mergify/bp/jazzy/pr-1185 branch March 17, 2026 03:50
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.

2 participants