feat(mmsg): batched datagram I/O via recvmmsg/sendmmsg - #41
Merged
Conversation
Add batched UDP send/receive behind default-off Linux build flags (MAFIANET_USE_RECVMMSG / MAFIANET_USE_SENDMMSG), plus a portable, unit-tested core. Portable core (MmsgBatch.h/.cpp), compiled and tested on every platform: - DriveBatchedSend: sendmmsg partial-send resume state machine. - SockaddrToSystemAddress: byte-order-correct sockaddr -> SystemAddress. - DispatchRecvBatch: fan a received batch to the event handler, freeing zero-length reads and the unused tail. Linux syscall wiring (guarded, inert by default): - RNS2_Berkley::RecvFromBatchedLoop drains the socket with one recvmmsg(MSG_WAITFORONE) per burst instead of one recvfrom per packet. - RNS2_Linux::SendBatch coalesces datagrams via sendmmsg; a loop-local RNS2SendBatch in ReliabilityLayer's resend loop defers only the transmit (encryption/simulation/metrics still run per datagram) and flushes at the loop's single exit. Tests: 11 hermetic unit tests for the core (Tests/Unit/MmsgBatchTests.cpp). The Linux path is compile-verified (gcc 13, both flags on); runtime validation on Linux via the integration suite is still pending.
|
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:
WalkthroughAdds Linux ChangesUDP batching
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)Batched receive flowsequenceDiagram
participant RNS2_Berkley
participant recvmmsg
participant DispatchRecvBatch
participant RNS2EventHandler
RNS2_Berkley->>recvmmsg: receive datagram batch
recvmmsg-->>RNS2_Berkley: return lengths and source addresses
RNS2_Berkley->>DispatchRecvBatch: dispatch received slots
DispatchRecvBatch->>RNS2EventHandler: dispatch valid slots and reclaim invalid slots
Batched send flowsequenceDiagram
participant ReliabilityLayer
participant RNS2SendBatch
participant RNS2_Linux
participant sendmmsg
ReliabilityLayer->>RNS2SendBatch: add prepared datagram
ReliabilityLayer->>RNS2SendBatch: flush update batch
RNS2SendBatch->>RNS2_Linux: send datagram batch
RNS2_Linux->>sendmmsg: transmit batch
sendmmsg-->>RNS2_Linux: return sent count or classified error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/CMakeLists.txt (1)
344-350: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the batched-UDP macros off the installed library consumers.
MAFIANET_USE_RECVMMSGandMAFIANET_USE_SENDMMSGare installed public options, butMAFIANET_USE_SENDMMSGcontrols theRNS2_Linux::SendBatchoverride and theRNS2SendBatchclass body. Consumers linking against the installed target only get the macro definitions the library was initialized with when building withMAFIANET_USE_*OFF, while installed consumers can include and useRNS2_Linux::SendBatchorRNS2SendBatchonly when the option was ON. Keep these flags internal, or expose them as target options and ensure exported consumers receive the matching config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/CMakeLists.txt` around lines 344 - 350, Keep MAFIANET_USE_RECVMMSG and MAFIANET_USE_SENDMMSG private to the library build in target_compile_definitions(${target_name}), and ensure the installed/exported target does not propagate them to consumers. Update the public RNS2_Linux::SendBatch and RNS2SendBatch configuration to remain consistent without requiring installed consumers to define these macros, or explicitly export matching option values if that API must remain conditional.
🧹 Nitpick comments (1)
Source/src/MmsgBatch.cpp (1)
13-32: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUnhandled address family leaves
outwith stale/uninitialized data.When
RAKNET_SUPPORT_IPV6 != 1andfrom.ss_family != AF_INET, neither branch executes, so*out(theRNS2RecvStruct::systemAddressof a possibly-reused struct) is left with whatever it held before.DispatchRecvBatchdoesn't gate dispatch on family validity, only onlens[i] > 0, so such a slot would still be handed toOnRNS2Recvwith a stale source address. This mirrors the existingRecvFromBlockingIPV4And6pattern by design, and a bound IPv4-only socket should never hand back a non-AF_INET address, so the practical exposure is low — but worth a defensive zero-init or assert given it silently misattributes a datagram's origin if it ever does trigger.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/src/MmsgBatch.cpp` around lines 13 - 32, Update SockaddrToSystemAddress to defensively initialize or otherwise invalidate *out before handling the address family, and explicitly handle unsupported families so no stale source address or port remains when neither AF_INET nor IPv6 applies. Preserve the existing IPv4 and IPv6 byte-order handling for supported families.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/include/mafianet/MmsgBatch.h`:
- Around line 116-125: Update RNS2SendBatch::Add to reject oversized datagrams
rather than clamping length to MAXIMUM_MTU_SIZE. Preserve the full payload
behavior of the non-batched send path by signaling failure before memcpy when
length exceeds the supported limit, and adjust the caller as needed to handle
that failure without incrementing count.
In `@Source/include/mafianet/socket2.h`:
- Around line 267-269: Update the conditional declaration of
RNS2Socket2::SendBatch in the socket2 header so it is available to consumers
whenever the implementation supports MAFIANET_USE_SENDMMSG, rather than relying
on a library-private compile definition. Align this visibility with the
corresponding CMakeLists.txt and MmsgBatch.h changes.
In `@Source/src/ReliabilityLayer.cpp`:
- Around line 2206-2210: Remove the per-call allocation in the UpdateInternal
send path by making the RNS2SendBatch storage reusable across ticks, such as
persistent member- or thread-owned storage initialized once and reused by
Add/Flush. Update the RNS2SendBatch construction near the send loop to use that
storage, while preserving the existing batch capacity and behavior and only
growing the buffer when necessary.
In `@Tests/Unit/MmsgBatchTests.cpp`:
- Line 21: Replace the direct arpa/inet.h include in MmsgBatchTests.cpp with
platform-conditional socket headers matching the library’s Windows handling: use
winsock2.h and ws2tcpip.h on MSVC/mingw, while retaining the appropriate POSIX
header elsewhere so htons and inet_pton remain available.
---
Outside diff comments:
In `@Source/CMakeLists.txt`:
- Around line 344-350: Keep MAFIANET_USE_RECVMMSG and MAFIANET_USE_SENDMMSG
private to the library build in target_compile_definitions(${target_name}), and
ensure the installed/exported target does not propagate them to consumers.
Update the public RNS2_Linux::SendBatch and RNS2SendBatch configuration to
remain consistent without requiring installed consumers to define these macros,
or explicitly export matching option values if that API must remain conditional.
---
Nitpick comments:
In `@Source/src/MmsgBatch.cpp`:
- Around line 13-32: Update SockaddrToSystemAddress to defensively initialize or
otherwise invalidate *out before handling the address family, and explicitly
handle unsupported families so no stale source address or port remains when
neither AF_INET nor IPv6 applies. Preserve the existing IPv4 and IPv6 byte-order
handling for supported families.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a84bf948-9853-4f37-8fef-812670c91381
📒 Files selected for processing (9)
Source/CMakeLists.txtSource/include/mafianet/MmsgBatch.hSource/include/mafianet/ReliabilityLayer.hSource/include/mafianet/socket2.hSource/src/MmsgBatch.cppSource/src/RakNetSocket2.cppSource/src/RakNetSocket2_Berkley.cppSource/src/ReliabilityLayer.cppTests/Unit/MmsgBatchTests.cpp
Address CI + review feedback on the batched I/O prototype: - Windows build: MmsgBatchTests.cpp used <arpa/inet.h>; guard the include (<ws2tcpip.h> on Windows) so the hermetic tests build cross-platform. - RNS2SendBatch::Add rejected oversized datagrams instead of silently clamping to the MTU, which diverged from the scalar send path and could ship truncated payloads in release builds. - RNS2SendBatch no longer heap-allocates ~95 KB per UpdateInternal call; it reuses a thread_local buffer (single update thread per RakPeer, flushed synchronously) so the send path stays allocation-free. - MAFIANET_USE_RECVMMSG/SENDMMSG are now PUBLIC compile definitions so consumers see the same conditional declarations the library was built with (no header/ABI mismatch).
… in CI Pre-landing review follow-ups on the batched datagram I/O prototype: - SendBatch returned a byte total from the portable base implementation but a datagram count from the sendmmsg override. Both now return a datagram count (and the error only when nothing was sent) by routing the scalar loop through the same DriveBatchedSend state machine. - SockaddrToSystemAddress silently left *out untouched for an address family the build cannot decode, so a recycled recv struct would attribute the datagram to its previous sender. It now returns bool, sets UNASSIGNED_SYSTEM_ADDRESS, and DispatchRecvBatch frees instead of dispatching. - RNS2SendBatch is non-copyable and asserts against a second live batch on the same thread; its shared buffer block moved from TLS to a lazily allocated per-thread block, so threads that never batch pay nothing. - The batched recv loop kept reallocating the whole slot batch on every recvmmsg failure. Slots are now held across failures with progressive back-off, so a persistent async error (ECONNREFUSED on a closed peer port) cannot spin the polling thread. - RNS2SendBatch is compiled unconditionally (it only needs the portable SendBatch), with unit tests for payload copying, batch-boundary flush, destructor backstop, and the oversized-datagram drop. - New linux-mmsg CI job builds and tests with both flags ON; the syscall paths and their reliability-layer wiring were previously never compiled anywhere. Verified: 98/98 (unit + integration) on Linux with MAFIANET_USE_RECVMMSG and MAFIANET_USE_SENDMMSG ON; 69/69 unit on macOS with both OFF.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/Unit/MmsgBatchTests.cpp (1)
57-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse MafiaNet containers in test scaffolding.
These
std::vectorusages conflict with the repository container convention. Replace them with an appropriateDataStructurescontainer.As per coding guidelines, use “custom MafiaNet data structure containers (DS_List, DS_Queue, DS_Map, DS_OrderedList, DS_MemoryPool) for consistency.”
Also applies to: 97-98, 309-312, 451-451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Unit/MmsgBatchTests.cpp` around lines 57 - 58, Replace the std::vector declarations in the MmsgBatchTests test scaffolding, including the referenced locations, with the appropriate DataStructures container types from the repository convention. Preserve each container’s required ordering and access behavior, and update any dependent operations or iteration as needed to compile with the selected MafiaNet containers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Tests/Unit/MmsgBatchTests.cpp`:
- Around line 57-58: Replace the std::vector declarations in the MmsgBatchTests
test scaffolding, including the referenced locations, with the appropriate
DataStructures container types from the repository convention. Preserve each
container’s required ordering and access behavior, and update any dependent
operations or iteration as needed to compile with the selected MafiaNet
containers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6c3ed44-6c07-4210-b8b1-ca5eae4d3eab
📒 Files selected for processing (9)
.github/workflows/build.ymlSource/CMakeLists.txtSource/include/mafianet/MmsgBatch.hSource/include/mafianet/ReliabilityLayer.hSource/include/mafianet/socket2.hSource/src/MmsgBatch.cppSource/src/RakNetSocket2.cppSource/src/RakNetSocket2_Berkley.cppTests/Unit/MmsgBatchTests.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- Source/include/mafianet/socket2.h
- Source/CMakeLists.txt
- Source/include/mafianet/ReliabilityLayer.h
- Source/src/RakNetSocket2.cpp
Pre-landing review follow-ups on the batched datagram I/O work. - RecvFromBatchedLoop refilled all MMSG_BATCH_MAX slots every pass and handed the whole array to DispatchRecvBatch, so a pass that received a single datagram still cost 64 allocs plus 63 frees -- each taking the event handler's pool mutex. Only the slots that actually received a datagram now change hands; the untouched tail carries over, putting the steady state at one alloc/free round trip per datagram, the same as the scalar path. - RNS2_Linux::SendBatch silently ignored RNS2_SendParameters::ttl, which the scalar Send() honours by bracketing the sendto with setsockopt(IP_TTL). sendmmsg has no per-message TTL, so a batch carrying one is now deferred to the portable base loop. Nothing sets ttl today (RNS2SendBatch always flushes with zero), so this closes a trap rather than a live bug. - RNS2SendBatch::Flush discarded SendBatch's result: a failed or partial sendmmsg dropped datagrams with no trace, where the scalar path at least reports its sendto failures. Both that shortfall and a dropped oversized datagram now emit RAKNET_DEBUG_PRINTF. Also drops the dead lens[] writes past the received count, corrects the comment claiming shutdown arrives on the recvmmsg error branch (the poke is a real datagram, so it surfaces as a normal pass), and switches to _FILE_AND_LINE_ to match the rest of the codebase. Tests: cover the DispatchRecvBatch clamp, the IPv6 decode branch of SockaddrToSystemAddress (and its IPv4-only-build rejection), TTL pass-through in the base SendBatch, and that RNS2SendBatch flushes with a zero TTL so the sendmmsg path is never deferred. Verified with the full suite on Linux with both flags on (102/102).
DriveBatchedSend treated any negative transmit() return as "stop", so a single undeliverable datagram silently discarded every datagram queued behind it -- the scalar Send() loop it replaced would have delivered those. Split the contract: 0 now means a transient socket-wide condition (stop, don't spin), negative means the message at that offset is itself undeliverable (drop it and continue). RNS2_Linux::SendBatch classifies errno accordingly, since sendmmsg funnels both through the same -1. Also: - Assert in RNS2SendBatch::Flush that SendBatch returned a datagram count rather than a byte total. That is the one contract the sendmmsg override cannot be unit-tested against, so check it at runtime. - Build the linux-mmsg CI job in Debug. It is the only job that compiles these paths, and RakAssert -- which guards the return contract, the RNS2SendBatch reentrancy invariant and the IPv4-only address branch -- is compiled out unless _DEBUG. - Make the IPv4-only branch of the sendmmsg address setup explicit instead of silently leaving msg_name null. - Cover the previously untested failure paths: a partially failing flush, a fully failing flush not resending its batch, per-message skip, and first-error propagation. - Ignore build-*/ and docs/superpowers/.
…no split Follow-up to the review of the batched datagram I/O branch. - RAKNET_DEBUG_PRINTF is a plain printf in every configuration, so the two dropped-datagram diagnostics in RNS2SendBatch wrote to stdout from the network thread in Release. A routine ENOBUFS burst on a loaded server turned them into unbounded, stdout-lock-taking spam inside the very loop batching exists to speed up. Both now go through MMSG_BATCH_DEBUG_PRINTF, which is debug-only -- matching the scalar send path, which ignores an individual sendto failure silently. - RecvFromBatchedLoop treated only n<0 as a failed recvmmsg pass. A pass returning 0 reset the backoff, dispatched nothing and slept not at all, i.e. spun the polling thread at 100%. Not expected under MSG_WAITFORONE, but the n<0 path was hardened against exactly this, so n<=0 takes it now. - Extracted the sendmmsg errno classification into ClassifySendmmsgErrno and unit-tested it. The transient (stop) vs. permanent (drop one, ship the rest) split is where getting it backwards loses traffic silently, and it was the one part of the glue no test covered -- the linux-mmsg CI job only ever exercises the happy path. - RNS2_Linux::SendBatch was guarded on MAFIANET_USE_SENDMMSG alone, but RNS2_Linux is the non-Windows socket class, so macOS and the BSDs compiled it too and failed on the missing mmsghdr/sendmmsg. Now requires __linux__ as well, so the flag is a no-op off Linux (portable base SendBatch) instead of a build break. Found by configuring the flags on macOS; no CI job covers that combination. Both suites pass in the default and the -DMAFIANET_USE_RECVMMSG=ON -DMAFIANET_USE_SENDMMSG=ON configurations (113/113 each).
Batching was an opt-in pair of CMake options (MAFIANET_USE_RECVMMSG / MAFIANET_USE_SENDMMSG), both OFF by default. That meant the code that actually ships was compiled by nothing: no developer machine, and only a single Debug CI job. Remove the options entirely and select the paths from a platform capability instead. #if defined(__linux__) #define MAFIANET_HAS_MMSG 1 #else #define MAFIANET_HAS_MMSG 0 #endif Defined 0/1 rather than defined/undefined so `#if MAFIANET_HAS_MMSG` fails loudly on a typo instead of silently taking the fallback branch. A plain `cmake ..` on Linux now gets batching; every other platform compiles the portable per-datagram paths, unchanged. Measured on the new 2560-message burst test (Release, strace -c): syscall per-datagram batched sendto 2937 32 sendmmsg 0 53 recvfrom 2623 0 recvmmsg 0 83 total 5522 168 Also in this change: * Tests that actually fill a batch. The existing suite sends ~1 datagram per tick, so RNS2SendBatch flushed a batch of one every time and the mid-loop flush at MMSG_BATCH_MAX was never reached. MmsgBatchLiveTests drives bursts far past that boundary and asserts the stream survives byte-for-byte and in order; 35 of 58 sendmmsg calls now return a full 64. Each message is its own checksum, so truncation, payload aliasing across the batch copy, and reordering each fail a distinct assertion. * CompactRecvSlots. The recv-slot carry-over in RecvFromBatchedLoop is the subtlest part of the loop and only ever ran on Linux, on its success path -- a clean pass consumes everything it allocated, so the survivor shift never executed. Extracted to a portable free function with unit tests, including a 500-pass fixed-seed stress proving no slot is leaked or double-freed. * ClassifySendmmsgErrno now treats ENOMEM as transient. sendmsg(2) lists it alongside ENOBUFS; classifying it as permanent dropped one healthy datagram per remaining slot and burned a syscall doing it. * RakNetSocket2::SendBatch contract doc corrected. It claimed a negative return when nothing went out, but the sendmmsg override returns 0 for transient failures. Documents the tri-state and the fact that the two implementations detect the transient case differently. * RNS2SendBatch::Slot / ThreadBatchLive moved out of line into MmsgBatch.cpp. As inline members of a public header holding thread_local state, a consumer instantiating RNS2SendBatch across a shared-library boundary got its own buffer block and its own reentrancy flag, silently disarming the guard. * CI: linux-mmsg -> linux-native, now a Debug/Release matrix. Release was previously never built with these paths compiled in. Verified on Linux (Debug and Release): 122/122 including 32 integration tests over loopback; macOS unit 90/90. New integration tests run 10x repeat clean.
The two MAFIANET_USE_*MMSG options were documented nowhere -- not README, not CLAUDE.md, not docs/ -- and they no longer exist. Replace the gap and the stale option tables with a description of what the feature does, that it is automatic on Linux, and what it measurably buys. * README.md, docs/getting-started/building.rst: drop the option rows, add a "Batched Datagram I/O" section with the measured syscall comparison. * docs/advanced/preprocessor-directives.rst: document MAFIANET_HAS_MMSG, why it is a capability rather than a build flag, why it is 0/1 rather than defined/undefined, and why the guard is needed on top of the platform check (RNS2_Linux also compiles on macOS and the BSDs). * CLAUDE.md: record that a build option here was removed deliberately and should not come back; note that a macOS/Windows build proves nothing about these paths, with the container command that does; and warn that MmsgBatchLiveTests must keep its counts above MMSG_BATCH_MAX or the batch-filling coverage silently disappears. No changelog entry: per the release procedure in CLAUDE.md, changelog entries are written at version-bump time, not per-PR.
Segfaultd
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/build.yml (1)
77-81: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not retry the entire CTest suite to produce a green build.
--repeat until-pass:3also applies toTests/Unit, which is labeledunitand described as hermetic/deterministic. Run the unit suite once and keep the retry only forTests/Integration.Suggested adjustment
- ctest --test-dir build \ - --output-on-failure \ - --repeat until-pass:3 \ - --timeout 600 \ - --output-junit junit.xml + ctest --test-dir build -L unit \ + --output-on-failure \ + --timeout 600 \ + --output-junit junit-unit.xml + ctest --test-dir build -L integration \ + --output-on-failure \ + --repeat until-pass:3 \ + --timeout 600 \ + --output-junit junit-integration.xml🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 77 - 81, Update the CTest invocation in the build workflow to remove the suite-wide --repeat until-pass:3 behavior, run the unit-labeled Tests/Unit suite exactly once, and retain retries only for the Tests/Integration suite. Preserve the existing failure output, timeout, and JUnit reporting options for both test runs.Source/src/RakNetSocket2.cpp (1)
354-357: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDisable batching when a Linux mmsg syscall is unavailable.
MAFIANET_HAS_MMSGis a compile-time capability, so non-Linux builds and older unpatched Linux deployments can still callrecvmmsg/sendmmsgwith no support in the runtime. HandleENOSYSand switch to the scalar implementations instead of treating it as an ordinary error/batch-backoff condition.
Source/src/RakNetSocket2.cpp#L354-L357: detectENOSYSand useRakNetSocket2::SendBatch(...)for the whole batch.Source/src/RakNetSocket2_Berkley.cpp#L590-L600: detectENOSYS, fall back torecv(...)for this receive pass, and avoid spinning/backing off on repeated unsupported-syscall failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/src/RakNetSocket2.cpp` around lines 354 - 357, Handle ENOSYS from sendmmsg in Source/src/RakNetSocket2.cpp#L354-L357 by calling RakNetSocket2::SendBatch(...) for the entire batch instead of classifying it as a normal error. In Source/src/RakNetSocket2_Berkley.cpp#L593-L607, handle ENOSYS from recvmmsg by falling back to recv(...) for the current receive pass and avoid retry spinning or backoff for repeated unsupported-syscall failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/advanced/preprocessor-directives.rst`:
- Around line 134-136: Update the documentation around MAFIANET_HAS_MMSG to
remove the claim that a misspelled macro fails loudly, unless an explicit
`#ifndef/`#error validation is added. Preserve the guidance to use `#if`
MAFIANET_HAS_MMSG rather than `#ifdef`.
In `@README.md`:
- Around line 89-95: Align the benchmark documentation in README.md (lines
89-95) and docs/getting-started/building.rst (lines 77-101) to one verified
workload, command, and result. Reconcile the conflicting 5555 → 184 and 5522 →
168 figures by selecting the authoritative verified result, then update both
tables consistently.
---
Outside diff comments:
In @.github/workflows/build.yml:
- Around line 77-81: Update the CTest invocation in the build workflow to remove
the suite-wide --repeat until-pass:3 behavior, run the unit-labeled Tests/Unit
suite exactly once, and retain retries only for the Tests/Integration suite.
Preserve the existing failure output, timeout, and JUnit reporting options for
both test runs.
In `@Source/src/RakNetSocket2.cpp`:
- Around line 354-357: Handle ENOSYS from sendmmsg in
Source/src/RakNetSocket2.cpp#L354-L357 by calling RakNetSocket2::SendBatch(...)
for the entire batch instead of classifying it as a normal error. In
Source/src/RakNetSocket2_Berkley.cpp#L593-L607, handle ENOSYS from recvmmsg by
falling back to recv(...) for the current receive pass and avoid retry spinning
or backoff for repeated unsupported-syscall failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b35c73a-59fa-470b-8ed1-7090e726c363
📒 Files selected for processing (16)
.github/workflows/build.yml.gitignoreCLAUDE.mdREADME.mdSource/CMakeLists.txtSource/include/mafianet/MmsgBatch.hSource/include/mafianet/ReliabilityLayer.hSource/include/mafianet/socket2.hSource/src/MmsgBatch.cppSource/src/RakNetSocket2.cppSource/src/RakNetSocket2_Berkley.cppSource/src/ReliabilityLayer.cppTests/Integration/MmsgBatchLiveTests.cppTests/Unit/MmsgBatchTests.cppdocs/advanced/preprocessor-directives.rstdocs/getting-started/building.rst
🚧 Files skipped from review as they are similar to previous changes (6)
- Source/include/mafianet/ReliabilityLayer.h
- .gitignore
- Source/src/MmsgBatch.cpp
- Source/include/mafianet/socket2.h
- Source/src/ReliabilityLayer.cpp
- Source/include/mafianet/MmsgBatch.h
MAFIANET_HAS_MMSG was not a build flag -- nothing could set it, it was derived purely from __linux__ -- but it read like one, and it was the last MAFIANET_* name attached to this feature after the CMake options were removed. An indirection over a single-token platform check does not earn its keep, and the rest of the codebase already guards platform code with plain #ifdef _WIN32 / #if defined(__APPLE__) rather than capability macros. Replaced by `#if defined(__linux__)` at all 9 sites. No behaviour change: the macro expanded to exactly this condition.
Addresses CodeRabbit review on #41. ENOSYS was the serious one. Batching is no longer optional at build time, so a kernel or sandbox that does not implement recvmmsg/sendmmsg had no way out: * sendmmsg: ClassifySendmmsgErrno treated ENOSYS as a permanent per-message error, so DriveBatchedSend dropped every datagram in the batch one at a time, burning a syscall on each. All outbound traffic lost. * recvmmsg: n<0 fell into the progressive back-off and retried forever against a syscall that can never succeed. No packet ever surfaced. Together that is total loss of networking on an otherwise healthy host -- reachable via seccomp profiles, gVisor, user-mode emulation, or a kernel older than the syscall. Now latched at runtime (MmsgUnavailable) and both paths fall back to the portable per-datagram code, which is strictly better than the compile-time switch it replaces because it needs no redeploy. Only ENOSYS latches the fallback. EPERM is deliberately excluded: some seccomp policies report it, but so does a firewall rejecting one destination, and disabling batching process-wide for that would be wrong. Also fixes an errno-clobber found while doing this: RecvFromBatchedLoop read errno after calling GetTimeUS(), which is itself a syscall. errno is only meaningful immediately after the call that failed, so it is now captured first. Other review items: * Benchmark figures in README.md and docs/getting-started/building.rst disagreed (5555/184 vs 5522/168) because they came from different runs of a nondeterministic test, and the PR description mixed columns from two runs into a table that did not even add up. Re-measured both arms three times, published the medians (5525 -> 178, ~31x), documented the reproduction command, and stated that counts vary run to run so the ratio is the result rather than the exact numbers. * CI ran the whole suite under --repeat until-pass:3. The unit suite is hermetic and deterministic by construction, so retrying it could only hide a genuine nondeterminism bug. Unit now runs exactly once; retries are reserved for the integration suite where they absorb loopback timing misses. * The docs claim that a misspelled MAFIANET_HAS_MMSG would "fail loudly" was wrong -- #if treats unknown identifiers as 0 -- but the macro and that text were already deleted in ecc730e. Verified on Linux, Debug and Release, in the new CI shape: unit 94/94 run once, integration 32/32. macOS unit 94/94. Sphinx builds clean.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/include/mafianet/socket2.h`:
- Around line 26-32: Update the batching documentation in
Source/include/mafianet/socket2.h lines 26-32 to state that delivery semantics
are equivalent but return-code or diagnostic reporting may differ; update
docs/getting-started/building.rst lines 71-75 to replace the claim that only
system-call count differs with wording that also acknowledges differing
transient-error reporting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6234c743-2bd7-490f-a567-f0e0f5cc7acb
📒 Files selected for processing (15)
.github/workflows/build.ymlCLAUDE.mdREADME.mdSource/CMakeLists.txtSource/include/mafianet/MmsgBatch.hSource/include/mafianet/ReliabilityLayer.hSource/include/mafianet/socket2.hSource/src/MmsgBatch.cppSource/src/RakNetSocket2.cppSource/src/RakNetSocket2_Berkley.cppSource/src/ReliabilityLayer.cppTests/Integration/MmsgBatchLiveTests.cppTests/Unit/MmsgBatchTests.cppdocs/advanced/preprocessor-directives.rstdocs/getting-started/building.rst
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/build.yml
- Source/include/mafianet/ReliabilityLayer.h
- README.md
- Source/src/RakNetSocket2_Berkley.cpp
- Source/CMakeLists.txt
- Source/src/ReliabilityLayer.cpp
- Tests/Unit/MmsgBatchTests.cpp
- Tests/Integration/MmsgBatchLiveTests.cpp
- Source/include/mafianet/MmsgBatch.h
Addresses CodeRabbit review on #41. The summary at the top of socket2.h said behaviour was "identical either way and only the number of system calls differs". That contradicted the SendBatch contract documented ~100 lines below in the same header, which spells out that the two implementations report a transient send failure differently: the sendmmsg override classifies errno and can return 0, while the portable Send() loop cannot read errno portably and reports every failure as permanent. The syscall count on a transient failure differs too -- the batched path stops, the portable one attempts a call per remaining datagram. Neither difference is reachable from application code (the value feeds a debug-only diagnostic in RNS2SendBatch::Flush, and both paths drop the datagrams for the reliability layer to resend), so the accurate claim is about delivery semantics, not about behaviour wholesale. Reworded in socket2.h and docs/getting-started/building.rst to say exactly that, and to point at the contract for the detail. Documentation only; no code change. Verified on Linux, Debug and Release: unit 94/94, integration 32/32. macOS unit 94/94. Sphinx builds clean.
Member
Author
|
Two findings from the review arrived as outside diff range items with no thread to reply to, so recording them here.
|
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
1 participant
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.
Batches UDP datagrams into single
recvmmsg/sendmmsgsystem calls on Linux, replacing onerecvfrom/sendtoper packet.Always on. No build flags, no macros, nothing to configure. The paths are guarded by a plain
#if defined(__linux__), so a plaincmake ..gets batching on Linux. Every other platform compiles the portable per-datagram paths. Delivery semantics are identical either way — the same datagrams arrive, in the same order, with the same reliability, and nothing differs that application code can observe. Two internals do differ: the syscall count, and how a transient send failure is reported insideSendBatch(the batched override classifieserrnoand can report "nothing sent, retry later"; the portable loop can't readerrnoportably throughSend()and calls every failure permanent). Either way the datagrams are dropped and the reliability layer resends.Impact
Measured on the 2560-message reliable-ordered burst in
Tests/Integration/MmsgBatchLiveTests.cpp(Linux, Release,strace -c, median of 3 runs per arm):sendtosendmmsgrecvfromrecvmmsg~31x fewer system calls. Up to 64 datagrams (
MMSG_BATCH_MAX) coalesce per call; 35 of 58sendmmsgcalls returned a full 64.Counts vary a percent or two between runs (congestion control decides how many datagrams are ready per tick), so the ratio is the result, not the exact figures. Reproduce:
strace -f -c -e trace=sendmmsg,sendto,recvmmsg,recvfrom \ ./build/Tests/IntegrationTests \ --gtest_filter='MmsgBatchLive.LargeReliableOrderedBurstArrivesIntactAndInOrder'The per-datagram column is the code every non-Linux platform runs; to reproduce it on Linux, flip the
#if defined(__linux__)batching guards to#if 0.(The wall-clock column is deliberately omitted —
straceinflates per-syscall cost, so this is proof of the count reduction, not a real-world speedup figure. A true benchmark needs an untraced run on real hardware.)Design
RNS2SendBatchaccumulates already-prepared datagrams (post-encryption, post loss-simulation) for one peer and flushes them at the reliability layer's single loop exit. Payloads are copied because the caller reuses its serialization buffer.RakNetSocket2::SendBatchis virtual with a portableSend()-loop default, overridden byRNS2_Linuxwithsendmmsg. Both return a datagram count, not a byte total.DriveBatchedSendhandles the partial-send resume.sendmmsgfunnels two opposite conditions through the same-1, soClassifySendmmsgErrnosplits them: transient socket-wide (EAGAIN/ENOBUFS/ENOMEM/EINTR) → stop and retry next tick; anything else → a property of that datagram, drop it and ship the rest. Treating a permanent per-message error as "stop" would silently discard every datagram after the bad one, which the scalar loop would have delivered.sendmmsghas no per-message TTL, so a batch carrying one defers to the baseSend()loop.__linux__guard is needed even insideRNS2_Linux, because that is the non-Windows socket class and also compiles on macOS and the BSDs.recvmmsg/sendmmsgreturnENOSYS(seccomp profile, gVisor, user-mode emulation, pre-3.0 kernel) the process latchesMmsgUnavailableand both paths revert to the portable per-datagram code. Without this, batching being mandatory would mean total loss of networking on such a host:ENOSYSwould classify as a permanent per-message error and drop every datagram one at a time, while the recv loop backed off forever. OnlyENOSYSlatches —EPERMis excluded because a firewall rejecting one destination reports it too.DriveBatchedSend,ClassifySendmmsgErrno,SockaddrToSystemAddress,CompactRecvSlots,RNS2SendBatch) compile and are unit-tested on every platform; only the syscall glue is behind the guard.Testing
58 unit tests covering the partial-send resume, the errno split in both directions, sockaddr decode/reject, recv-slot ownership and carry-over, and the flush boundary.
3 live integration tests (
MmsgBatchLiveTests.cpp) that actually fill batches — the rest of the suite sends ~1 datagram per tick, soRNS2SendBatchflushed a batch of one every time and the mid-loop flush atMMSG_BATCH_MAXwas never reached:recvmmsgslot)Each message is its own checksum, so truncation, payload aliasing across the batch copy, and reordering each fail a distinct assertion.
Verified:
CI job⚠️ If branch protection pins the old
linux-mmsg→linux-native, now a Debug/Release matrix. The unit suite runs once (hermetic and deterministic — retrying it could only hide a real nondeterminism bug);--repeat until-pass:3is reserved for the integration suite. Debug is the only config whereRakAssertfires; Release is what ships and exercises the backstops the asserts hide.linux-mmsgcheck name, it needs updating.Notes for reviewers
CLAUDE.md, changelog entries are written at version-bump time, not per-PR.Known limitations
Honest scope of what has and hasn't been proven:
ENOBUFSand partial-sendmmsgare exercised against fakes in unit tests; a clean loopback run never provokes them.Recommendation: staged rollout — enable on one server, watch it, expand. The remaining risk is exposure to real network conditions, which only production provides.