Skip to content

fix(cpp): stop TextFormat iteration if output stream fails#26237

Closed
jschroedr wants to merge 4 commits into
protocolbuffers:mainfrom
jschroedr:bugfix/#23288
Closed

fix(cpp): stop TextFormat iteration if output stream fails#26237
jschroedr wants to merge 4 commits into
protocolbuffers:mainfrom
jschroedr:bugfix/#23288

Conversation

@jschroedr

@jschroedr jschroedr commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

The TextFormat::Printer currently continues to iterate over fields and repeated elements even after the underlying ZeroCopyOutputStream returns false. This results in unnecessary CPU usage (reflection and formatting) for large messages.

This change:

  • Adds a virtual failed() method to BaseTextGenerator.
  • Implements failed() in the concrete TextFormat::Printer::TextGenerator to expose the state of the output stream.
  • Adds short-circuit checks to exit early if the generator has failed (TextFormat::Printer::PrintMessage, TextFormat::Printer::PrintField, TextFormat::Printer::PrintShortRepeatedField)

Fixes: #23288

@honglooker

Copy link
Copy Markdown
Contributor

Looks like you're out of date. Would you mind:

  • rebasing
  • adding a unit test

Thanks

@jschroedr
jschroedr requested a review from a team as a code owner March 12, 2026 01:13
@jschroedr
jschroedr requested review from anandolee and removed request for a team March 12, 2026 01:13
@jschroedr

Copy link
Copy Markdown
Contributor Author

Hi @honglooker - confirming I've added a test showing how the update prevents TextFormat from printing beyond the point of failure.

More specifically, the test stops exactly at the failure limit of 64 bytes and makes one attempt after failure, turning O(N) work into O(1) work once the output destination is reached.

Note: I've included the verification in a standalone test file to ensure a clean build environment independent of the current experimental features in the main test suite.

@github-actions github-actions Bot added untriaged auto added to all issues by default when created. and removed wait for user action labels Mar 12, 2026
@honglooker

Copy link
Copy Markdown
Contributor

Huh, I recall the CLA being ✅ a few days ago - now it's red? You'll need to sign that before we proceed, thank you (maybe different email?)

@honglooker honglooker added cla: no and removed untriaged auto added to all issues by default when created. c++ labels Mar 12, 2026
@jschroedr

Copy link
Copy Markdown
Contributor Author

All set now @honglooker

@jschroedr

Copy link
Copy Markdown
Contributor Author

Just updated with the base branch - please let me know if anything else is needed.

@JasonLunn JasonLunn added c++ text format 🅰️ safe for tests Mark a commit as safe to run presubmits over labels May 15, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 15, 2026
Comment thread src/google/protobuf/text_format.cc Outdated
…cuiting early to prevent unnecessary work when the generator is in a failed state
@jschroedr

Copy link
Copy Markdown
Contributor Author

Feedback has been addressed @JasonLunn - thank you for your time reviewing and please let me know if you need anything else.

@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 16, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 16, 2026
@JasonLunn
JasonLunn removed the request for review from anandolee May 17, 2026 00:40
@JasonLunn JasonLunn added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed wait for user action labels May 17, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 17, 2026
protobuf-team-bot added a commit that referenced this pull request May 18, 2026
robot-piglet pushed a commit to userver-framework/userver that referenced this pull request Jun 25, 2026
* Optimized @ref ugrpc::ToLimitedDebugString and logging middlewares for Protobuf v35+ (to be precise, [this](protocolbuffers/protobuf#26237) PR is required).
   * At the same time, this may introduce a performance regression for older Protobuf versions. Please measure on your workloads. In extreme cases, disabling the logging middleware or setting `msg-size-log-limit: 0` should help.

userver/grpc/benchmarks:

```
BenchCustomLimit                     6156 ns         6155 ns       112481
BenchCustomLimitExceptionBased      21486 ns        21485 ns        32497
```
commit_hash:444433baa658f140354be31611d1a3e8698d623a
Malevrovich pushed a commit to Malevrovich/userver that referenced this pull request Jul 3, 2026
* Optimized @ref ugrpc::ToLimitedDebugString and logging middlewares for Protobuf v35+ (to be precise, [this](protocolbuffers/protobuf#26237) PR is required).
   * At the same time, this may introduce a performance regression for older Protobuf versions. Please measure on your workloads. In extreme cases, disabling the logging middleware or setting `msg-size-log-limit: 0` should help.

userver/grpc/benchmarks:

```
BenchCustomLimit                     6156 ns         6155 ns       112481
BenchCustomLimitExceptionBased      21486 ns        21485 ns        32497
```
commit_hash:444433baa658f140354be31611d1a3e8698d623a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interrupt work of TextFormat::Printer if io::ZeroCopyOutputStream::Next return false

3 participants