From db42c6cbbac10d7d7cc50d7e8ee38945e0fe9528 Mon Sep 17 00:00:00 2001 From: Matt Morley Date: Sun, 29 Mar 2026 20:41:32 -0700 Subject: [PATCH] [wpinet] Add mDNS discovery tests and fix mDNS JNI bugs (#8682) In https://github.com/wpilibsuite/allwpilib/issues/8681 we discovered that multicast service types need to be valid (end with _tcp or _udp), or else errors are silently swallowed. Let's make our C++ unit test use a valid name and also check that it works. I think if we should/shouldn't do this is up for debate still. I also discovered two bugs in the JNI code that lead to incorrect results being returned - Return array index was always 0 - Use of JLocal for the return value seems to mean that the array will always be NULL in java --- .github/workflows/bazel.yml | 2 +- .github/workflows/cmake.yml | 6 +++ .github/workflows/gradle.yml | 6 ++- .github/workflows/sanitizers.yml | 9 +++- tsan_suppressions.txt | 1 + wpinet/src/main/native/cpp/jni/WPINetJNI.cpp | 6 +-- .../net/MulticastServiceAnnouncerTest.java | 53 +++++++++++++++++++ wpinet/src/test/native/cpp/MulticastTest.cpp | 20 +++++-- 8 files changed, 92 insertions(+), 11 deletions(-) create mode 100644 tsan_suppressions.txt create mode 100644 wpinet/src/test/java/org/wpilib/net/MulticastServiceAnnouncerTest.java diff --git a/.github/workflows/bazel.yml b/.github/workflows/bazel.yml index 83b61a1485..07688cd55a 100644 --- a/.github/workflows/bazel.yml +++ b/.github/workflows/bazel.yml @@ -73,7 +73,7 @@ jobs: - name: Install apt dependencies if: matrix.os == 'ubuntu-24.04' - run: sudo apt-get update && sudo apt-get install -y libgl1-mesa-dev libx11-dev libxcursor-dev libxi-dev libxinerama-dev libxrandr-dev + run: sudo apt-get update && sudo apt-get install -y libgl1-mesa-dev libx11-dev libxcursor-dev libxi-dev libxinerama-dev libxrandr-dev avahi-daemon - if: matrix.os == 'ubuntu-24.04' uses: bazel-contrib/setup-bazel@0.15.0 diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index 640ff44544..21d414cb62 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -37,6 +37,12 @@ jobs: if: runner.os == 'Linux' run: sudo apt-get update && sudo apt-get install -y libopencv-dev libopencv-java ninja-build + - name: Setup avahi-daemon + if: runner.os == 'Linux' + run: | + sudo service dbus start + sudo avahi-daemon -D + - name: Install dependencies (macOS) if: runner.os == 'macOS' run: brew install opencv ninja diff --git a/.github/workflows/gradle.yml b/.github/workflows/gradle.yml index 2dc520151d..d6d2ea08db 100644 --- a/.github/workflows/gradle.yml +++ b/.github/workflows/gradle.yml @@ -60,7 +60,11 @@ jobs: with: image: ${{ matrix.container }} options: -v ${{ github.workspace }}:/work -w /work -e ARTIFACTORY_PUBLISH_USERNAME -e ARTIFACTORY_PUBLISH_PASSWORD -e GITHUB_REF -e CI - run: ./gradlew build --build-cache -PbuildServer -PskipJavaFormat ${{ matrix.build-options }} ${{ env.EXTRA_GRADLE_ARGS }} + # Start avahi-daemon and build + run: | + service dbus start + avahi-daemon -D + ./gradlew build --build-cache -PbuildServer -PskipJavaFormat ${{ matrix.build-options }} ${{ env.EXTRA_GRADLE_ARGS }} env: ARTIFACTORY_PUBLISH_USERNAME: ${{ secrets.ARTIFACTORY_USERNAME }} ARTIFACTORY_PUBLISH_PASSWORD: ${{ secrets.ARTIFACTORY_PASSWORD }} diff --git a/.github/workflows/sanitizers.yml b/.github/workflows/sanitizers.yml index 5f34e3333c..7fe6ca1739 100644 --- a/.github/workflows/sanitizers.yml +++ b/.github/workflows/sanitizers.yml @@ -22,7 +22,7 @@ jobs: ctest-flags: "-E 'wpilibc'" - name: tsan cmake-flags: "-DCMAKE_BUILD_TYPE=Tsan" - ctest-env: "TSAN_OPTIONS=second_deadlock_stack=1" + ctest-env: "TSAN_OPTIONS=second_deadlock_stack=1:suppressions=$GITHUB_WORKSPACE/tsan_suppressions.txt" ctest-flags: "-E 'cscore|cameraserver'" - name: ubsan cmake-flags: "-DCMAKE_BUILD_TYPE=Ubsan" @@ -33,7 +33,7 @@ jobs: container: wpilib/roborio-cross-ubuntu:2025-24.04 steps: - name: Install Dependencies - run: sudo apt-get update && sudo apt-get install -y libopencv-dev libopencv-java clang-18 ninja-build + run: sudo apt-get update && sudo apt-get install -y libopencv-dev libopencv-java clang-18 ninja-build avahi-daemon - name: Install sccache uses: mozilla-actions/sccache-action@v0.0.9 @@ -46,6 +46,11 @@ jobs: SCCACHE_WEBDAV_USERNAME: ${{ secrets.ARTIFACTORY_USERNAME }} SCCACHE_WEBDAV_PASSWORD: ${{ secrets.ARTIFACTORY_PASSWORD }} + - name: Setup avahi-daemon + run: | + sudo service dbus start + sudo avahi-daemon -D + - name: build working-directory: build run: cmake --build . --parallel $(nproc) diff --git a/tsan_suppressions.txt b/tsan_suppressions.txt new file mode 100644 index 0000000000..69c8346225 --- /dev/null +++ b/tsan_suppressions.txt @@ -0,0 +1 @@ +deadlock:libdbus-1.so.3 diff --git a/wpinet/src/main/native/cpp/jni/WPINetJNI.cpp b/wpinet/src/main/native/cpp/jni/WPINetJNI.cpp index ca5eba3741..75f5adc995 100644 --- a/wpinet/src/main/native/cpp/jni/WPINetJNI.cpp +++ b/wpinet/src/main/native/cpp/jni/WPINetJNI.cpp @@ -330,9 +330,10 @@ Java_org_wpilib_net_WPINetJNI_getMulticastServiceResolverData return serviceDataEmptyArray; } - JLocal returnData{ - env, env->NewObjectArray(allData.size(), serviceDataCls, nullptr)}; + jobjectArray returnData = + env->NewObjectArray(allData.size(), serviceDataCls, nullptr); + size_t index = 0; for (auto&& data : allData) { JLocal serviceName{env, MakeJString(env, data.serviceName)}; JLocal hostName{env, MakeJString(env, data.hostName)}; @@ -340,7 +341,6 @@ Java_org_wpilib_net_WPINetJNI_getMulticastServiceResolverData wpi::util::SmallVector keysRef; wpi::util::SmallVector valuesRef; - size_t index = 0; for (auto&& txt : data.txt) { keysRef.emplace_back(txt.first); valuesRef.emplace_back(txt.second); diff --git a/wpinet/src/test/java/org/wpilib/net/MulticastServiceAnnouncerTest.java b/wpinet/src/test/java/org/wpilib/net/MulticastServiceAnnouncerTest.java new file mode 100644 index 0000000000..665eed4782 --- /dev/null +++ b/wpinet/src/test/java/org/wpilib/net/MulticastServiceAnnouncerTest.java @@ -0,0 +1,53 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.net; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.net.UnknownHostException; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.Test; + +class MulticastServiceAnnouncerTest { + @Test + void emptyText() throws InterruptedException, UnknownHostException { + final String serviceName = "Foaksdfjasklfj"; + final String serviceType = "_wpinotxt._tcp"; + final int port = 12345; + + try (MulticastServiceAnnouncer announcer = + new MulticastServiceAnnouncer(serviceName, serviceType, port); + MulticastServiceResolver resolver = new MulticastServiceResolver(serviceType)) { + assertTrue(announcer.hasImplementation() && resolver.hasImplementation()); + + announcer.start(); + resolver.start(); + + List allData = new ArrayList<>(); + + for (int i = 0; i < 10; i++) { + ServiceData[] data = resolver.getData(); + if (data == null) { + continue; + } + + allData.addAll(List.of(data)); + + if (!allData.isEmpty()) { + break; + } + + Thread.sleep(1000); + } + + assertFalse(allData.isEmpty(), "Expected at least one service entry, but got none"); + + resolver.stop(); + announcer.stop(); + } + } +} diff --git a/wpinet/src/test/native/cpp/MulticastTest.cpp b/wpinet/src/test/native/cpp/MulticastTest.cpp index 06724f0790..79232f8502 100644 --- a/wpinet/src/test/native/cpp/MulticastTest.cpp +++ b/wpinet/src/test/native/cpp/MulticastTest.cpp @@ -3,11 +3,10 @@ // the WPILib BSD license file in the root directory of this project. #include -#include #include #include #include -#include +#include #include @@ -17,7 +16,7 @@ TEST(MulticastServiceAnnouncerTest, EmptyText) { const std::string_view serviceName = "TestServiceNoText"; - const std::string_view serviceType = "_wpinotxt"; + const std::string_view serviceType = "_wpinotxt._tcp"; const int port = std::rand(); wpi::net::MulticastServiceAnnouncer announcer(serviceName, serviceType, port); wpi::net::MulticastServiceResolver resolver(serviceType); @@ -26,7 +25,20 @@ TEST(MulticastServiceAnnouncerTest, EmptyText) { announcer.Start(); resolver.Start(); - std::this_thread::sleep_for(std::chrono::seconds(1)); + std::vector allData; + + for (int i = 0; i < 15; i++) { + // GetData gives me new data since last time. This smoketest is just + // looking for -any- response + allData = resolver.GetData(); + if (!allData.empty()) { + break; + } + + std::this_thread::sleep_for(std::chrono::seconds(1)); + } + + ASSERT_GT(allData.size(), 0ul); resolver.Stop(); announcer.Stop();