Skip to content

Add optional FlatBuffers messages and code generation - #3558

Open
AnDiXL wants to merge 3 commits into
apache:masterfrom
AnDiXL:review/flatbuffers-msg-3196
Open

AnDiXL wants to merge 3 commits into
apache:masterfrom
AnDiXL:review/flatbuffers-msg-3196

Conversation

@AnDiXL

@AnDiXL AnDiXL commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Related discussion: #2354.

This is the message/codegen follow-up to #3196, opened at @wwbmmm's request. It submits the two commits already shared there as a focused, independently reviewable PR.

Background and related work:

Thanks to @Q1ngbo for the original work, and to @Spicy-cream for the continued interest in FlatBuffers support and collaboration around these follow-ups.

What is changed and the side effects?

Changed:

  • 3f2550c6: add IOBuf-backed Message, allocator-aware MessageBuilder, stable service/method descriptors, optional CMake/Make/Bazel integration, documentation and regression tests.
  • 6e5a3b32: fix generated header-guard collisions and relocation, and make standalone codegen acceptance use the appropriate Protobuf includes, link target and C++ standard.
  • Message derives from NonreflectableMessage<Message>; existing protobuf-facing Protocol callback signatures remain unchanged.
  • The standalone brpc_flatc binding generator uses the upstream FlatBuffers parser rather than requiring a compiler fork.

Scope and side effects:

  • FlatBuffers support is disabled by default.
  • This is message construction and generated in-process dispatch, not an fb_rpc transport or a network RPC example. The RPC follow-up will carry those separately.
  • No measured performance improvement is claimed.

Validation

Recorded checks, not rerun for PR creation:

  • Exact PR head (6e5a3b32), macOS/CMake: 21 message tests and both flatbuffers_codegen_acceptance and flatbuffers_codegen_runtime passed, with no failures or skips.
  • Related full message/RPC snapshots were also validated on Linux with CMake, Make and Bazel. This is integration evidence, not a separate Linux validation of this reduced two-commit PR.
  • Full-repository tests, sanitizer/performance results and hosted CI for this new PR are not claimed here.

Build and usage instructions are in docs/en/flatbuffers.md and tools/flatbuffers/README.md.


Check List:

  • Focused build and test validation is recorded above.
  • Message and code-generation regression tests are included.
  • Build instructions and ownership/verification limitations are documented.
  • Hosted CI for this PR has completed.

xulei25 added 2 commits September 21, 2026 01:02
Add IOBuf-backed messages, allocator-aware builders and stable service
method descriptors. Provide a standalone binding generator using the
upstream FlatBuffers parser rather than requiring a compiler fork.

Keep the feature disabled by default in CMake, Make and Bazel. Cover
message ownership, framing, schema verification, method IDs and generated
name collisions with regression tests.

Build on the SingleIOBuf foundation merged in apache#3062 and the
message-construction work in apache#3196. Network transport and
Channel/Server integration remain separate.
Keep service header guards distinct across punctuation, case and equal
schema basenames in different namespaces without embedding checkout
paths. Test standalone headers, both include orders and relocation.

Select C++14 or C++17 according to Protobuf and use its exported include
paths and link target for standalone acceptance. Rebuild and pass both
codegen acceptance tests before committing.

Build on message construction in 3f2550c and RPC support in 338fe83,
continuing apache#3196 and apache#3197 over the SingleIOBuf
foundation from apache#3062.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The ownership-sensitive allocator implementation, generated public API, and three build-system integrations warrant final human review.

Review effort: Balanced
Findings: None

What changed in this PR

Adds optional FlatBuffers-backed messages, service descriptors, and standalone service-binding generation without introducing network transport.

Changes:

  • Implements move-only messages, builders, allocators, and descriptors.
  • Adds brpc_flatc generation and documentation.
  • Integrates optional support and tests across Make, CMake, and Bazel.
File Description
WORKSPACE Pins FlatBuffers for Bazel.
MODULE.bazel Adds the Bzlmod FlatBuffers archive.
BUILD.bazel Adds conditional sources and dependency.
CMakeLists.txt Adds optional CMake support.
Makefile Includes FlatBuffers sources conditionally.
config.h.in Defines the feature macro.
config_brpc.sh Adds Make configuration support.
docs/​en/​flatbuffers.md Documents construction and descriptor APIs.
src/​brpc/​flatbuffers/​message.h Declares messages, builders, and allocator.
src/​brpc/​flatbuffers/​message.cpp Implements storage and serialization behavior.
src/​brpc/​flatbuffers/​service.h Declares service descriptor interfaces.
src/​brpc/​flatbuffers/​service.cpp Implements descriptor validation and lookup.
tools/​flatbuffers/​CMakeLists.txt Builds the standalone generator.
tools/​flatbuffers/​README.md Documents generator usage and contracts.
tools/​flatbuffers/​brpc_flatc.cpp Generates service bindings from schemas.
test/​Makefile Adds conditional Make tests.
test/​CMakeLists.txt Adds FlatBuffers CMake tests.
test/​BUILD.bazel Adds conditional Bazel tests.
test/​flatbuffers_message.fbs Defines message test data.
test/​brpc_flatbuffers_unittest.cpp Tests messages, allocators, and descriptors.
test/​flatbuffers_codegen/​echo.fbs Defines code-generation fixtures.
test/​flatbuffers_codegen/​CMakeLists.txt Configures code-generation tests.
test/​flatbuffers_codegen/​acceptance.cmake Tests generated-code acceptance.
test/​flatbuffers_codegen/​runtime.cpp Tests generated dispatch behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread docs/en/flatbuffers.md
@@ -0,0 +1,123 @@
# FlatBuffers messages

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you also add a chinese version of this document?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could you also add a chinese version of this document?

Of course, I will submit it tomorrow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added docs/cn/flatbuffers.md and linked it from the English document. This Chinese version is scoped to the message/codegen part of this PR; the RPC/example documentation remains in the follow-up draft.

@wwbmmm

wwbmmm commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

I think this PR is more comprehensive than #3196 . @Q1ngbo What do you think of this PR? Does it conflict with your previous work on flatbuffer?

Comment thread MODULE.bazel Outdated
Comment on lines +83 to +95
# runtime_cc and flatc do not need FlatBuffers' gRPC module dependency, which
# would otherwise conflict with brpc's BoringSSL version even when disabled.
# Keep the archive and checksum in sync with WORKSPACE.
flatbuffers_http_archive = use_repo_rule(
'@bazel_tools//tools/build_defs/repo:http.bzl',
'http_archive',
)
flatbuffers_http_archive(
name = 'com_github_google_flatbuffers',
sha256 = 'b9c2df49707c57a48fc0923d52b8c73beb72d675f9d44b2211e4569be40a7421',
strip_prefix = 'flatbuffers-25.2.10',
urls = ['https://github.com/google/flatbuffers/archive/refs/tags/v25.2.10.tar.gz'],
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is different about bazel_dep(name = "flatbuffers", version = "25.2.10")?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

bazel_dep(name = "flatbuffers", version = "25.2.10") is not equivalent here. The BCR module for FlatBuffers 25.2.10 imports gRPC and several JS/Go/Swift tooling dependencies for the full upstream build, while this PR only needs //:runtime_cc and //:flatc. Pulling that module would also bring gRPC's dependency graph, including another BoringSSL version, into Bzlmod resolution even when BRPC_WITH_FLATBUFFERS is false.

I kept the checksum-pinned http_archive and expanded the comment in MODULE.bazel to make this explicit.

Add a Chinese version of the FlatBuffers message and codegen guide for the focused message PR.

Explain why the Bzlmod path keeps the checksum-pinned FlatBuffers archive instead of bazel_dep: the BCR module imports gRPC and language/tooling dependencies while bRPC only needs runtime_cc and flatc.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Make-generated headers can shadow CMake outputs, and hosted CI currently skips all FlatBuffers test paths.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Medium severity Generate Make header outside source tree to prevent stale shadowing

test/​Makefile:245

Generating this header directly in test/ contaminates the other build paths. Because brpc_flatbuffers_unittest.cpp uses a quoted include, a header left by Make is searched before CMake's ${CMAKE_CURRENT_BINARY_DIR} output, so a later CMake build can silently compile a stale header (or fail its FlatBuffers version assertion); the file is also not covered by the generated-file patterns in .gitignore. Generate it in an ignored Make build directory and add that directory to this target's include path so the source tree can never shadow CMake's generated output.

Low severity Enable FlatBuffers tests and codegen coverage in CI

test/​BUILD.bazel:260

This test is incompatible unless --define=BRPC_WITH_FLATBUFFERS=true is passed, but none of the current GitHub Actions Bazel, CMake, or Make jobs enables FlatBuffers (and no job invokes the standalone codegen suite). Consequently all new runtime and generator behavior is skipped by hosted CI despite the repository's otherwise comprehensive C++ test runs. Add an enabled CI job that installs matching FlatBuffers headers/flatc and runs both this unit test and the standalone codegen acceptance/runtime tests.

@AnDiXL

AnDiXL commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

For the hosted FlatBuffers ON matrix: I intentionally kept it in the follow-up draft #3559 rather than this message/codegen PR. #3559 adds .github/workflows/flatbuffers-on.yml plus the validation runner and covers the FlatBuffers-enabled CMake/Make/Bazel paths together with the RPC/example changes. This PR keeps the smaller message/codegen scope.

@wwbmmm wwbmmm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[replied by brpc-oncall robot] I agree with Copilot's two findings (the Make-generated header can shadow CMake's output, and hosted CI does not exercise any of the new FlatBuffers tests). My own remaining concerns are: the runtime binds to FlatBuffers internals without any version validation, Message::MergeFrom aborts the process rather than reporting an error, and the receive path's size/verification obligations are not stated. Please also confirm the service.h dispatch contract is what the RPC transport PR will consume, since it becomes public API here.

Comment thread CMakeLists.txt

set(WITH_FLATBUFFERS_VAL "0")
if(WITH_FLATBUFFERS)
find_path(FLATBUFFERS_INCLUDE_DIR NAMES flatbuffers/flatbuffers.h)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[replied by brpc-oncall robot] WITH_FLATBUFFERS is validated only by the existence of flatbuffers/flatbuffers.h, but the runtime does not use FlatBuffers purely through its public API: src/brpc/flatbuffers/message.cpp reaches into builder internals (buf_, buf_.swap_allocator(), scratch_push_small(), string_pool, minalign_), and SlabAllocator asserts on the exact allocate()/reallocate_downward() size bookkeeping (only one live allocation, old_size == _capacity). Bazel pins 25.2.10 while CMake and config_brpc.sh accept any installed version, so with a different FlatBuffers an unsupported version fails deep in compilation or aborts inside the allocator. Please add a compile-time guard in src/brpc/flatbuffers/message.h (e.g. #if !defined(FLATBUFFERS_VERSION_MAJOR) || FLATBUFFERS_VERSION_MAJOR < X -> #error) and document the supported range, so users get a clear diagnostic instead of an obscure failure.


// Failure leaves the old message unchanged. A fragmented or unaligned
// payload is copied into aligned storage; aligned contiguous input is shared.
bool parse_msg_from_iobuf(const butil::IOBuf& buf, size_t msg_size,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[replied by brpc-oncall robot] parse_msg_from_iobuf (and therefore ParseFbFromIOBUF) accepts any msg_size below FLATBUFFERS_MAX_BUFFER_SIZE and, for fragmented or insufficiently aligned input, copies that many bytes into a fresh allocation before anything is verified. Once the RPC transport lands this length comes from an untrusted peer. Please state the caller contract here (and in docs/en/flatbuffers.md): the caller must bound msg_size with its own max-message-size setting and must call Verify<T>() before reading, so the receive-side limit is not left implicit.

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.

4 participants