Skip to content

Abstract httplib from plugin API, vendor as build farm fallback#347

Merged
bburda merged 18 commits intomainfrom
fix/vendor-cpp-httplib
Apr 5, 2026
Merged

Abstract httplib from plugin API, vendor as build farm fallback#347
bburda merged 18 commits intomainfrom
fix/vendor-cpp-httplib

Conversation

@bburda
Copy link
Copy Markdown
Collaborator

@bburda bburda commented Apr 3, 2026

Pull Request

Summary

Removes cpp-httplib from the public plugin API so only ros2_medkit_gateway depends on it. Plugins now use thin PluginRequest/PluginResponse wrappers and declare routes declaratively via get_routes() instead of imperatively registering on httplib::Server.

This also vendors cpp-httplib 0.14.3 in the gateway package with a VENDORED_DIR cmake fallback, fixing the Humble build farm failure where the system package (0.10.x) is too old.


Issue


Type

  • Bug fix
  • New feature or tests
  • Breaking change
  • Documentation only

Testing

  • Full colcon build (13 packages) passes
  • 2422 unit tests pass (0 failures)
  • New unit test RegisterRoutesWrapsPluginHandlers verifies end-to-end route registration and PluginRequest/PluginResponse wrapping
  • New unit tests for PluginRequest (path_param, header, path, body) and PluginResponse (send_json, send_error)
  • Integration test updated for demo plugin response format change (plaintext -> JSON)
  • Linters pass

Checklist

  • Breaking changes are clearly described (and announced in docs / changelog if needed)
  • Tests were added or updated if needed
  • Docs were updated if behavior or public API changed

Breaking changes

  • PLUGIN_API_VERSION bumped 4 -> 5
  • GatewayPlugin::register_routes() and get_route_descriptions() removed, replaced by get_routes() returning vector<PluginRoute>
  • PluginContext::send_json() and send_error() static methods removed, now instance methods on PluginResponse
  • Plugin route handlers receive PluginRequest/PluginResponse instead of httplib::Request/httplib::Response
  • Plugin packages no longer need libcpp-httplib-dev, libssl-dev, or medkit_find_cpp_httplib() in CMake

Copilot AI review requested due to automatic review settings April 3, 2026 09:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the gateway plugin HTTP surface to remove cpp-httplib from the public plugin API by introducing PluginRequest/PluginResponse wrappers and a declarative GatewayPlugin::get_routes() route model, and adds a vendored cpp-httplib fallback to address Humble/Jammy build farm constraints.

Changes:

  • Replace plugin route registration (register_routes) with get_routes() returning PluginRoute handlers using PluginRequest/PluginResponse.
  • Vendor cpp-httplib (intended 0.14.3) and extend medkit_find_cpp_httplib() with a VENDORED_DIR fallback tier.
  • Update affected plugins, unit tests, integration tests, and documentation to the v5 plugin API.

Reviewed changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/gateway_plugin.hpp Introduces PluginRoute + get_routes() API.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_http_types.hpp Adds PluginRequest/PluginResponse wrapper types.
src/ros2_medkit_gateway/src/plugins/plugin_http_types.cpp Implements wrappers via HandlerContext::send_json/send_error.
src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp Wraps/installs plugin routes onto the HTTP server via get_routes().
src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp Hides httplib from the header by using an opaque server pointer.
src/ros2_medkit_gateway/src/http/rest_server.cpp Adapts route registration callsite to new register_routes(void*) signature.
src/ros2_medkit_gateway/src/plugins/plugin_context.cpp / include/.../plugin_context.hpp Switches validate_entity_for_route to PluginRequest/PluginResponse and removes static response helpers.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_types.hpp Bumps PLUGIN_API_VERSION to 5.
src/ros2_medkit_gateway/CMakeLists.txt Uses medkit_find_cpp_httplib(VENDORED_DIR ...), adds wrapper source, installs vendored header.
src/ros2_medkit_cmake/cmake/ROS2MedkitCompat.cmake Adds VENDORED_DIR fallback support to medkit_find_cpp_httplib().
src/ros2_medkit_cmake/README.md Documents the new vendored fallback parameter.
src/ros2_medkit_gateway/src/vendored/cpp_httplib/LICENSE Adds vendored dependency license.
src/ros2_medkit_gateway/src/vendored/cpp_httplib/httplib.h Adds vendored cpp-httplib header (intended fallback).
src/ros2_medkit_gateway/test/test_plugin_manager.cpp Updates tests for get_routes() and adds an end-to-end wrapping test.
src/ros2_medkit_gateway/test/test_plugin_http_types.cpp Adds unit tests for PluginRequest/PluginResponse.
src/ros2_medkit_gateway/test/demo_nodes/test_gateway_plugin.cpp Updates demo plugin to new route/response API (JSON ping).
src/ros2_medkit_integration_tests/test/features/test_plugin_vendor_extensions.test.py Updates integration assertion to expect JSON response.
src/ros2_medkit_plugins/ros2_medkit_graph_provider/src/graph_provider_plugin.cpp Migrates to get_routes() + wrappers.
src/ros2_medkit_plugins/ros2_medkit_graph_provider/include/.../graph_provider_plugin.hpp Updates plugin interface override to get_routes().
src/ros2_medkit_plugins/ros2_medkit_graph_provider/test/test_graph_provider_plugin.cpp Updates route tests and provides wrapper stubs for non-gateway-linked tests.
src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt / package.xml Removes httplib/openssl deps from plugin; re-adds for route tests only.
src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/src/topic_beacon_plugin.cpp Migrates to get_routes() + wrappers.
src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/test/test_topic_beacon_plugin.cpp Updates stubs and context signature for new HTTP types.
src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/CMakeLists.txt / package.xml / include/...hpp Removes httplib/openssl deps and old route APIs.
src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/src/param_beacon_plugin.cpp Migrates to get_routes() + wrappers.
src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/test/test_param_beacon_plugin.cpp Updates stubs and context signature for new HTTP types.
src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/CMakeLists.txt / package.xml / include/...hpp Removes httplib/openssl deps and old route APIs.
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/src/{procfs,systemd,container}_plugin.cpp Migrates to get_routes() + wrappers and response helpers on PluginResponse.
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/CMakeLists.txt / package.xml / design/index.rst Removes httplib/openssl deps and updates design docs for new API.
docs/tutorials/plugin-system.rst Updates tutorial to API v5 + new route/response model.
docs/config/server.rst Updates plugin lifecycle docs to get_routes().
docs/troubleshooting.rst Updates troubleshooting reference to get_routes().
Comments suppressed due to low confidence (1)

src/ros2_medkit_gateway/CMakeLists.txt:62

  • With the new VENDORED_DIR fallback, gateway_lib may compile against the vendored cpp-httplib while test targets still include <httplib.h> from the system (e.g., 0.10.x on Humble). That can lead to ABI/type mismatches (tests constructing httplib::Request/Server with one header version while gateway_lib is built against another) and even missing APIs like wait_until_ready(). Consider forcing all targets in this package (especially gtests) to build against the same cpp_httplib_target include directory (e.g., by linking cpp_httplib_target PUBLIC from gateway_lib or adding it explicitly to each test target) so the vendored fallback is exercised safely on the build farm.
medkit_find_cpp_httplib(VENDORED_DIR "${CMAKE_CURRENT_SOURCE_DIR}/src/vendored/cpp_httplib")

# Find OpenSSL (required by jwt-cpp for RS256 and optional TLS support)
find_package(OpenSSL REQUIRED)

# Enable OpenSSL support for cpp-httplib SSLServer
# This enables the httplib::SSLServer class for HTTPS support
add_compile_definitions(CPPHTTPLIB_OPENSSL_SUPPORT)

@bburda bburda self-assigned this Apr 3, 2026
@bburda bburda requested a review from mfaferek93 April 3, 2026 10:03
@bburda bburda marked this pull request as draft April 3, 2026 10:14
@bburda bburda force-pushed the fix/vendor-cpp-httplib branch 2 times, most recently from 89acca5 to 5c27522 Compare April 3, 2026 12:24
@bburda bburda marked this pull request as ready for review April 3, 2026 12:24
@bburda bburda requested a review from Copilot April 3, 2026 12:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 41 out of 42 changed files in this pull request and generated 2 comments.

@bburda bburda force-pushed the fix/vendor-cpp-httplib branch 5 times, most recently from 49a025a to 07e7fed Compare April 4, 2026 10:38
bburda added 13 commits April 4, 2026 12:39
Add httplib.h v0.14.3 (MIT) to gateway vendored directory.
Update medkit_find_cpp_httplib() macro to accept VENDORED_DIR parameter
as fallback when system package is too old (Ubuntu 22.04 ships 0.10.x).

Fixes #341
Thin wrappers over httplib::Request/Response that hide the HTTP library
from plugin code. Implementation compiled in gateway_lib; plugins
resolve symbols at runtime from the gateway process.
Plugins now declare routes via get_routes() returning PluginRoute structs
instead of imperatively registering on httplib::Server. The gateway wraps
httplib types in PluginRequest/PluginResponse before calling handlers.

Removes httplib from the public plugin API. PLUGIN_API_VERSION bumped 4->5.
Replace register_routes(httplib::Server&, prefix) with get_routes() returning
vector<PluginRoute>. Handler now uses PluginRequest/PluginResponse instead of
httplib types directly. Remove httplib and OpenSSL build/package dependencies
from plugin target; keep only for test target which does end-to-end HTTP testing.
Update FakePluginContext::validate_entity_for_route and add PluginRequest/
PluginResponse stubs in the test.
Replace register_routes + get_route_descriptions with get_routes() returning
vector<PluginRoute>. Handler now uses PluginRequest/PluginResponse instead of
httplib types directly. Remove httplib and OpenSSL build/package dependencies.
Update MockPluginContext::validate_entity_for_route and stubs in test.
Replace register_routes + get_route_descriptions with get_routes() returning
vector<PluginRoute>. Handler now uses PluginRequest/PluginResponse instead of
httplib types directly. Remove httplib and OpenSSL build/package dependencies.
Update MockPluginContext::validate_entity_for_route and stubs in test.
Replace register_routes(httplib::Server&, prefix) with get_routes() returning
vector<PluginRoute> in procfs, systemd, and container plugins. Handlers now use
PluginRequest/PluginResponse instead of httplib types directly. Remove httplib
and OpenSSL build/package dependencies from all three plugin targets and tests.
- Fix missing '/' between api_prefix and route pattern in
  PluginManager::register_routes (produced /api/v1apps/... instead
  of /api/v1/apps/...)
- Fix integration test expecting plaintext 'pong' after demo plugin
  was changed to return JSON
- Update plugin-system.rst, server.rst, troubleshooting.rst, and
  linux_introspection design doc to reference new get_routes() API
  instead of removed register_routes()
- Update documented API version from 4 to 5
- Remove dead code get_all_route_info() (zero callers)
- Add unit test verifying PluginManager route wrapping end-to-end
Eviction sorted by completed_at timestamps with millisecond precision,
causing non-deterministic eviction order when fast executions complete
within the same millisecond. Use monotonic creation_seq counter instead
to guarantee FIFO eviction regardless of completion timing.
No longer needed - the gateway vendors cpp-httplib 0.14.3 as a fallback
when the system package is too old. The medkit_find_cpp_httplib() macro
with VENDORED_DIR handles this automatically.
Remove manual source install instructions for Humble - the vendored
fallback handles this automatically. Update troubleshooting section
and cmake error message accordingly.
@bburda bburda force-pushed the fix/vendor-cpp-httplib branch from 07e7fed to e1edd61 Compare April 4, 2026 10:41
No longer needed - the gateway vendors cpp-httplib 0.14.3 as a fallback
when the system package is too old. Removes the conditional git clone
step and the libcpp-httplib-dev skip-key from rosdep.
@bburda bburda force-pushed the fix/vendor-cpp-httplib branch 2 times, most recently from 5afcbab to 3c93976 Compare April 4, 2026 20:04
- Use RelWithDebInfo instead of Debug (2-3x faster under TSan)
- Set halt_on_error=0 so TSan reports races without killing the process
- Add history_size=4 for better race reports
- Run ctest with -j1 to avoid parallel test interference under TSan
@bburda bburda force-pushed the fix/vendor-cpp-httplib branch from 3c93976 to 26a1b0d Compare April 5, 2026 06:13
bburda added 3 commits April 5, 2026 08:27
- Fix dangling reference: capture full_pattern by value in httplib
  handler lambda (was captured by ref to loop-scoped local)
- Restore type safety: register_routes takes httplib::Server& with
  forward declaration instead of void*
- Return const string refs from PluginRequest::path()/body()
- Fix doxygen: remove httplib type names from opaque pointer docs
- Fix stale register_routes references in gateway_params.yaml,
  introspection_provider.hpp, merge_pipeline.cpp
Timer cancel() does not wait for an in-flight callback to finish on
Humble. If the callback is running when the destructor resets the
publisher, the callback dereferences a null SharedPtr -> SIGSEGV.

Add a mutex that the callback locks during execution. The destructor
acquires it after cancel() to wait for any in-flight callback before
resetting members. Callback also checks for null publisher defensively.

Applied to rpm_sensor as an experiment; other demo nodes will follow
if this resolves the recurring Humble SIGSEGV failures.
…Notifier::shutdown

Classical lost-wakeup race between shutdown() and worker_loop():

1. worker locks queue_mutex_
2. worker checks predicate (flag=false, queue empty) -> false
3. shutdown() CAS flag -> true (NOT holding queue_mutex_)
4. shutdown() notify_one() - worker NOT yet enqueued on CV
5. worker enters wait(lock): atomic unlock+enqueue+sleep
6. worker sleeps forever; main blocks in worker_thread_.join()

Even though shutdown_flag_ uses seq_cst atomics, the worker's predicate
check and entry into wait() are not atomic with respect to the flag
modification. The notify can arrive before the worker is enqueued.

Fix: briefly acquire queue_mutex_ between setting the flag and notifying.
This guarantees the worker is either still outside its critical section
(will observe the new flag on the next lock) or already enqueued on the
CV (notify_one will wake it).

Manifested as a TSan-specific hang in DoubleShutdownIsSafe (worker spawn
+ immediate shutdown hits the race window).
@mfaferek93 mfaferek93 self-requested a review April 5, 2026 19:04
@bburda bburda merged commit e560fa7 into main Apr 5, 2026
14 checks passed
@bburda bburda deleted the fix/vendor-cpp-httplib branch April 5, 2026 19:05
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.

[BUG] Humble build farm fails: cpp-httplib 0.10.x too old

3 participants