Migrate QuicCommunication onto shared ba-quic-lib transport - #69
Open
danprudky wants to merge 12 commits into
Open
Migrate QuicCommunication onto shared ba-quic-lib transport#69danprudky wants to merge 12 commits into
danprudky wants to merge 12 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe QUIC integration now uses BAQuicLib and QUIC transport migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant QuicCommunication
participant QuicClient
participant ExternalPeer
QuicCommunication->>QuicClient: initialize()
QuicCommunication->>QuicClient: connect()
QuicClient->>ExternalPeer: QUIC handshake
ExternalPeer-->>QuicClient: connection established
QuicClient-->>QuicCommunication: onConnected()
QuicCommunication->>QuicClient: send(serialized ExternalClient)
QuicClient-->>QuicCommunication: onBytesReceived()
QuicCommunication->>QuicClient: disconnect()
QuicClient-->>QuicCommunication: onDisconnected()
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jiriskuta
force-pushed
the
BAF-1723/Migrate-onto-the-QUIC-lib
branch
from
July 23, 2026 09:33
4b133fb to
4626ff1
Compare
…nsport - replace the hand-rolled MsQuic client wrapper (registration/configuration/connection/stream callbacks) with bringauto::quic::QuicClient from the new ba-quic-lib dependency, removing ~250 lines of manual msquic plumbing from QuicCommunication - keep the existing ConnectionState state machine and the outbound queue/dedicated sender thread, since ExternalConnection enqueues the Fleet-protocol Connect message before the QUIC handshake completes and relies on the queue to defer the send until CONNECTED - delete QuicSettingsParser; quic-settings JSON is now passed straight through to bringauto::quic::QuicSettings::fromJson, which also warns on unrecognized keys instead of silently ignoring them - swap BA_PACKAGE_LIBRARY(msquic)/FIND_PACKAGE(msquic CONFIG REQUIRED) for cmake/FindBAQuicLib.cmake + FIND_PACKAGE(BAQuicLib REQUIRED), matching the module-mode resolution convention (in-scope target -> system package -> FetchContent) - init bringauto::quic::Logger alongside the app's own logger in main.cpp, otherwise the library's internal logs are silently dropped - reorder QuicCommunication's members so quicClient_ outlives the condvars/atomics it can still call back into during ~QuicClient()'s blocking RegistrationClose BREAKING CHANGE: the QUIC transport now always uses one unidirectional stream per message; the "stream-mode" config key (and bidirectional streams) is no longer supported and is ignored with a warning if present.
jiriskuta
force-pushed
the
BAF-1723/Migrate-onto-the-QUIC-lib
branch
from
July 23, 2026 09:53
4626ff1 to
ac348ba
Compare
The v0.1.0 tag now exists on quic-lib, so pin the FetchContent GIT_TAG to it instead of tracking the master branch head, making the build reproducible. Removes the now-obsolete TODO.
…chContent When module-gateway configures with BRINGAUTO_INSTALL=ON (its CMDEF packaging), FetchContent pulls ba-quic-lib in via add_subdirectory, so it inherits the flag and runs its own install(EXPORT ba-quic-lib-targets). That fails at generate time: the exported ba-quic-lib target links msquic PUBLIC, but msquic (also a FetchContent subdir target) is in no export set, and CMake forbids exporting a target whose public dependency isn't exported too. module-gateway links ba-quic-lib statically in-tree and never consumes its install/export, so shadow BRINGAUTO_INSTALL=OFF around the fetch, mirroring the existing BRINGAUTO_TESTS shadow.
… via ba-quic-lib PR #69 removed BA_PACKAGE_LIBRARY(msquic v2.5.6) + FIND_PACKAGE(msquic CONFIG), which left ba-quic-lib to resolve msquic itself -- its FindBAMsquic falls through to FetchContent and builds msquic + vendored quictls/OpenSSL from source. That source build needs Perl FindBin.pm (absent on the fedora44 CI image) and libatomic/libnuma, none of which the prebuilt-package path ever required. Restore the prebuilt msquic and resolve it BEFORE FIND_PACKAGE(BAQuicLib): ba-quic-lib's FindBAMsquic returns early on an already-defined 'msquic' target (if(TARGET msquic) return()), so it now reuses the prebuilt package -- no source build, matching how this repo (and every other msquic consumer) always did it.
…t-msquic wiring) Reverts 8b05090. Per direction, msquic is owned by ba-quic-lib (the new shared transport lib) and consumed transitively; module-gateway no longer declares its own msquic package or FIND_PACKAGE(msquic). This is consistent with PR #69, which moved QuicCommunication off direct msquic.h onto ba-quic-lib's QuicClient, so MG has no direct msquic dependency of its own. Everything else in MG's CMLIB-based build is unchanged.
v0.1.1 resolves msquic from the prebuilt bacpack package instead of building it from source, so the fedora44 CI build no longer needs the msquic source-build toolchain (Perl/FindBin, libnuma).
- Replace the hardcoded "ModuleGateway" string with bringauto::quic::kLoggerId.id when initializing the ba-quic-lib logger instance - The literal id didn't match the id ba-quic-lib actually registers under, so its logs could go uninitialized/swallowed despite the init() call
vbartak
reviewed
Jul 28, 2026
vbartak
left a comment
vbartak
left a comment
There was a problem hiding this comment.
Review
Direction is right — the msquic plumbing does not belong in QuicCommunication, and −473 net lines is a real win. Comment quality in the new code is unusually good.
Two things I'd want resolved before merge, marked 🔴 inline:
senderThread_threading regression — it's written from an msquic worker thread with no synchronization, andonDisconnected()dropped the oldrequest_stop(), so the nextonConnected()implicitly joins the previous session's thread from the transport's own worker.- Dependency resolution bypasses the
BA_PACKAGE_LIBRARYconvention —FetchContentfrom private GitLab at configure time, andmsquicno longer flows throughBA_PACKAGE_DEPS_IMPORTED.
Severity legend: 🔴 major · 🟠 mid · 🔵 minor · ⚪ nit
Not anchorable to a diff line
- 🔵
CLAUDE.md:77is stale — still states config is parsed by "SettingsParser(andQuicSettingsParserfor QUIC-specific fields)". This PR deletesQuicSettingsParser. - 🟠 Zero test coverage.
buildEndpointConfigandbuildQuicSettingsare pure static functions over astd::unordered_map<std::string, std::string>— the cheapest possible unit tests, andbuildQuicSettingsis exactly where config-parsing behavior changed (widened key set, new warning path, JSON re-interpretation). The gtest suite gained nothing from a 743-line deletion.
Done well
resources/config/README.mddocuments both the removedstream-modeand the new pass-through contract, andquic_example.jsonwas updated to match.- Unrecognized keys now warn instead of vanishing — strictly better than the old four-key parser.
- The reconnect queue-drain and the notify-under-guarding-mutex fixes survived the refactor intact.
- The
main.cppnote explaining whybringauto::quic::Logger::init()is required is the comment that saves the next reader an hour.
…ency, CMake robustness, test coverage - #3664157558/#3664157568 senderThread_: guard with senderThreadMutex_, explicitly stop+join from onDisconnected()/stop() instead of relying on implicit jthread destruction racing an msquic worker callback - #3664157621 initializeConnection(): bounded wait for onDisconnected() + CAS instead of a hard NOT_CONNECTED store on handshake timeout - #3664157629 sendViaQuicClient(): null-check quicClient_ before dereferencing - #3664157632 constructor: widen catch to std::exception (QuicSettings::fromJson is third-party) - #3664157646 onShutdownInitiatedByPeer(): notify outboundCv_ so senderLoop unwinds without waiting for onDisconnected() - #3664157649 buildEndpointConfig(): drop no-op static_cast<uint16_t> on settings.port - #3664157641 buildQuicSettings(): document the string-vs-numeric re-typing caveat (no functional change, matches existing serializeToJson shape) - #3664157587 FindBAQuicLib.cmake: use string(FIND) instead of MATCHES (regex) for the build-dir cache-entry check - #3664157592 FindBAQuicLib.cmake: FORCE-write the BRINGAUTO_TESTS/BRINGAUTO_INSTALL cache entries instead of relying on CMP0077=NEW in ba-quic-lib's own scope - #3664157614 FindBAQuicLib.cmake: drop include_guard(GLOBAL), redundant with the existing TARGET check and unsafe across scopes - #3664157581 CMakeLists.txt: resolve BAQuicLib before the BRINGAUTO_GET_PACKAGES_ONLY early-return, and copy FindBAQuicLib.cmake into the Dockerfile's cache-builder stage, so ba-quic-lib is pre-fetched during Docker cache warming - #3664157625 add QuicCommunicationTests covering buildQuicSettings/buildEndpointConfig (known key, unknown key, non-numeric value, endpoint-key exclusion, empty settings) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ceptions and lookups - Catch nlohmann::json::exception instead of std::exception in QuicCommunication's constructor, since buildQuicSettings only ever throws JSON-related errors - Reuse the outer `expected` variable in initializeConnection instead of redeclaring it, fixing a shadowing warning - Give ExternalConnectionSettings::protocolSettings a transparent hash/ equality (TransparentStringHash + std::equal_to<>) so lookups by std::string_view avoid an allocation; update getProtocolSettingsString and the ConfigMock/QuicCommunicationTests mocks to match the new map type Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t CMake error
- FindBAQuicLib.cmake now builds the FetchContent GIT_REPOSITORY URL with
$ENV{BA_GITLAB_TOKEN_URI}, matching transparent-module's PR #11 fix, so
CI can clone the private quic-lib GitLab repo
- Resolve libbringauto_logger before FIND_PACKAGE(BAQuicLib REQUIRED) in
CMakeLists.txt: ba-quic-lib vendors its own FindBALogger.cmake that
defines a partial bringauto_logger::bringauto_logger shim if the target
doesn't exist yet, which made the later real libbringauto_logger CONFIG
import fail with "Some (but not all) targets in this export set were
already defined"
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…scope An unqualified `friend class QuicCommunicationTests;` inside a namespace either refers to (or, if undeclared, silently introduces) that name in the enclosing namespace, not the global-scope test fixture the friend declaration was meant to grant access to. clang++-17 (the tested test-suite compiler) and this build's g++ both rejected the private buildQuicSettings/buildEndpointConfig calls from QuicCommunicationTests as a result. Forward-declare the fixture at global scope and qualify the friend declaration with `::` so it resolves there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
InternalServerTests.FiftyClients occasionally crashed the entire test binary
("Subprocess aborted") instead of just failing, whenever the module handler
missed its response timeout for one of the 50 simulated devices under CI load.
The server closes that device's connection, but ClientForTesting::sendMessage
used the throwing write_some() overload; a subsequent write to the now-closed
socket threw an uncaught boost::system::system_error ("Broken pipe"), which
terminate()d the process. Switch both sendMessage overloads to the non-throwing
error_code overload + ASSERT_FALSE(er), matching the pattern already used by
receiveMessage/connectSocket in this same file, so this now fails the one
assertion instead of killing the whole suite.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breaking change: the QUIC transport now always uses one unidirectional stream per message — the "stream-mode" config key (and bidirectional streams) is no longer supported and is ignored with a warning if present.
Dependency: requires the ba-quic-lib branch project-migration-features to be merged first — this MR's FetchContent currently tracks the master branch head and will not build until those changes land there.
Summary by CodeRabbit
New Features
Documentation
stream-modesetting and reflect the new keys/behavior.