Repository navigation
Conversation
0934686 to
48177aa
Compare
|
Hi @chenBright @wwbmmm @zchuango @darion-yaphet, thanks again for all the reviews on #3558! #3559 is the last PR in the FlatBuffers series: the optional Could you please take a look when you have time? Any feedback is welcome. Thanks! |
Add the optional fb_rpc transport with length-delimited metadata and stable method IDs, and integrate it with Channel, Controller and Server without changing the existing protobuf Protocol callback signatures. Keep request buffers immutable across retries and concurrent calls, and manage service ownership, asynchronous completion and concurrency limits. Reject invalid frames and unsupported authentication, compression, checksums and streams. The feature stays disabled by default in CMake, Make and Bazel. Add 33 protocol tests covering the wire format, service lifecycle, malformed input and real RPC round trips.
Add a client/server example built from a generated service, with a bounded loopback smoke test: 13 verified replies, 2 expected schema rejections and a clean shutdown. It is a functional check, not a benchmark. The example takes an installed FlatBuffers-enabled bRPC and writes the code generated by flatc and brpc_flatc into its own build directory.
Add a runner that builds and tests the FlatBuffers-enabled configuration with CMake, Make or Bazel and rejects empty, filtered or skipped test runs. Add its self-tests and a GitHub Actions workflow that runs it on Linux and macOS when FlatBuffers-related paths change.
Add Chinese and English guides covering how to enable FlatBuffers, the dependency and ABI requirements, building the runtime, running the example and its smoke test, the network RPC API and the FRPC wire format. Update the generator README for the RPC example.
48177aa to
8cbb17b
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
_options.protocolis configured as a protocol name (e.g. \"fb_rpc\" / \"baidu_std\"), but here… · New_options.protocolis configured as a protocol name (e.g. \"fb_rpc\" / \"baidu_std\"), but here… · Newstd::unordered_setis used in this file, but the FlatBuffers include block only adds<map>in… · New
What changed in this PR
This PR adds optional FlatBuffers RPC (“fb_rpc”) transport support on top of the existing FlatBuffers message/service generation work, plus an end-to-end example and a dedicated FlatBuffers-ON validation workflow.
Changes:
- Integrates
fb_rpcas a new built-inProtocolwith Channel/Controller/Server support, service registration, and concurrency integration. - Adds comprehensive FlatBuffers protocol/unit tests and wires them into CMake/Make/Bazel test builds.
- Adds a generated client/server example with a bounded smoke test, updated documentation (EN/CN), and a GitHub Actions “FlatBuffers ON” gate runner.
| File | Description |
|---|---|
| tools/flatbuffers/README.md | Updates generator README to point to runtime enablement + fb_rpc usage and verification guidance. |
| test/brpc_flatbuffers_protocol_unittest.cpp | Adds dedicated protocol-level and lifecycle tests for fb_rpc. |
| test/brpc_channel_unittest.cpp | Avoids colliding with newly assigned built-in protocol IDs in channel tests. |
| test/Makefile | Ensures FlatBuffers protocol tests are excluded when FlatBuffers is OFF and builds/link rules include the new test. |
| test/CMakeLists.txt | Adds brpc_flatbuffers_protocol_unittest to CMake test targets when FlatBuffers is ON. |
| test/BUILD.bazel | Adds Bazel cc_test for brpc_flatbuffers_protocol_unittest and excludes it from generic globbed tests. |
| src/brpc/server.h | Introduces FlatBuffers service registration API and internal method property plumbing (guarded by BRPC_WITH_FLATBUFFERS). |
| src/brpc/server.cpp | Implements FlatBuffers service registry, versioning/stat exposure, concurrency limiter hookup, and conflict checks with protobuf services. |
| src/brpc/policy/flatbuffers_protocol.h | Declares the fb_rpc protocol hook points (parse/verify/serialize/pack/process). |
| src/brpc/policy/flatbuffers_protocol.cpp | Implements FRPC framing, request/response processing, and async completion lifecycle for FlatBuffers RPC. |
| src/brpc/options.proto | Reserves built-in PROTOCOL_FLATBUFFERS_RPC = 30. |
| src/brpc/global.cpp | Registers fb_rpc as a built-in protocol when FlatBuffers is enabled. |
| src/brpc/details/server_private_accessor.h | Exposes FlatBuffers method lookup to protocol code via server private accessor. |
| src/brpc/details/controller_private_accessor.h | Adds private setter for FlatBuffers method descriptor on Controller. |
| src/brpc/controller.h | Adds Controller::flatbuffers_method() accessor and stores FlatBuffers method descriptor pointer. |
| src/brpc/controller.cpp | Clears FlatBuffers method context in Controller::ResetPods(). |
| src/brpc/channel.h | Extends Channel to implement flatbuffers::RpcChannel and adds FBCallMethod. |
| src/brpc/channel.cpp | Routes FlatBuffers calls via FBCallMethod, adds entry validation, and adjusts tracing method-name selection. |
| src/brpc/CMakeLists.txt | Ensures flatbuffers_protocol.cpp is excluded when FlatBuffers is OFF. |
| example/benchmark_fb/smoke.py | Adds bounded end-to-end smoke test orchestration for generated FB client/server. |
| example/benchmark_fb/server.cpp | Adds example FlatBuffers server (protocol fb_rpc, service registration, graceful shutdown). |
| example/benchmark_fb/echo.fbs | Defines example schema and stable wire ID for the RPC method. |
| example/benchmark_fb/client.cpp | Adds example FlatBuffers client driving generated stub + verifies response schema/attachments. |
| example/benchmark_fb/README.md | Documents building/running the example and what the smoke test validates. |
| example/benchmark_fb/CMakeLists.txt | Adds standalone CMake build for the example + codegen steps and smoke CTest. |
| docs/en/flatbuffers.md | Expands docs to cover fb_rpc enablement, full verification steps, and limitations. |
| docs/cn/flatbuffers.md | Mirrors EN documentation updates for fb_rpc enablement and verification steps. |
| README_cn.md | Links to the CN FlatBuffers RPC guide. |
| README.md | Links to the EN FlatBuffers RPC guide. |
| Makefile | Excludes flatbuffers_protocol.cpp from Make builds when FlatBuffers is OFF. |
| CMakeLists.txt | Updates WITH_FLATBUFFERS option description to include RPC support. |
| BUILD.bazel | Includes flatbuffers_protocol.cpp in FlatBuffers-enabled Bazel source patterns. |
| .gitignore | Stops ignoring .github/ to keep workflows/scripts discoverable. |
| .github/workflows/flatbuffers-on.yml | Adds a CI workflow that runs the FlatBuffers-ON gates across CMake/Make/Bazel on Linux/macOS. |
| .github/scripts/test_flatbuffers_on.py | Adds self-tests validating the gate script’s report parsing and safety checks. |
| .github/scripts/flatbuffers-on.py | Adds an “ON runner” that builds deps, runs unit/codegen/example tests, and rejects empty/skipped runs. |
Review effort: Lite
| return; | ||
| } | ||
| cntl->set_used_by_rpc(); | ||
| const bool is_fb_protocol = (_options.protocol == PROTOCOL_FLATBUFFERS_RPC); |
| const int64_t start_send_us = butil::cpuwide_time_us(); | ||
| std::string method_name; | ||
| if (_get_method_name) { | ||
| if (_get_method_name && is_fb == is_fb_protocol) { |
| // Keep ownership with the caller until the entire record is published. | ||
| auto record = std::make_unique<FlatBuffersServiceMap::Service>(service, sd); | ||
| std::unordered_set<std::string> method_names; |
| Method* FindMethodByFullName(const butil::StringPiece& fullname) const { | ||
| for (const auto& service : services) { | ||
| for (const auto& method : service.second->methods) { | ||
| if (method.second->property.method->full_name() == fullname) { | ||
| return method.second.get(); | ||
| } | ||
| } | ||
| } | ||
| return nullptr; | ||
| } |
|
[replied by brpc-oncall robot] Thanks for splitting the RPC increment out of #3197 and for the very thorough documentation. Because this PR adds a new built-in protocol together with public API on
Note: Copilot's three findings above look like false positives to me. Thanks! |
| const uint32_t attachment_size = Load32(meta + 8); | ||
| const bthread_id_t correlation_id = {Load64(meta + 12)}; | ||
| Controller* cntl = nullptr; | ||
| const int rc = bthread_id_lock(correlation_id, reinterpret_cast<void**>(&cntl)); |
There was a problem hiding this comment.
bthread_id_lock() validates a process-wide correlation ID, but does not verify that the response arrived on the socket used by that request. A response carrying another in-flight RPC's valid ID can therefore modify and complete that RPC.
Please follow the existing protobuf response handlers: check that cntl is non-null and call does_response_match_request_socket() before modifying the Controller or response. Unlock and discard the message when validation fails, and add regression coverage for mismatched sockets and stale retry responses.
What problem does this PR solve?
This continues the RPC work in #3197 on top of #3558, which is now merged. #3558 added the FlatBuffers messages, builders, service descriptors and the
brpc_flatcgenerator; this PR adds the optionalfb_rpctransport on top of it.The branch is rebased onto master after #3558 and contains only the four RPC commits.
What is changed and the side effects?
Add FlatBuffers RPC protocol support: optionalfb_rpctransport, Channel/Controller/Server integration, service lifecycle handling, attachments, retries and concurrency, with 33 protocol tests.Add a generated FlatBuffers RPC example: generated client/server example with a bounded loopback smoke test. This is functional validation, not a performance benchmark.Add explicit FlatBuffers ON validation: an ON validation runner, its self-tests and a GitHub Actions workflow.Document FlatBuffers setup and example checks: Chinese and English setup, dependency, ABI and example-verification documentation.The transport uses explicit metadata lengths, so optional fields can be appended later while the known prefix stays readable. Existing protobuf-facing
Protocolcallback signatures are unchanged.Side effects and limits:
output/include. The ON runner and the docs now install the runtime into a prefix before building the example.Validation
Run on the rebased head
48177aa2:LINK_SO=ON)python3 .github/scripts/test_flatbuffers_on.pyThe example smoke verifies 13 replies, 2 expected schema rejections and a clean shutdown.
Not covered by these runs: Make and Bazel ON modes (left to CI), sanitizer runs and performance benchmarks.
Check List: