Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduces Printer::Shrink #434

Closed

Conversation

nicolasnoble
Copy link
Contributor

Introduces Printer::Shrink, to let the user avoid an assert if its output stream will disappear before our destructor.

The idea here is that if you do this:

std::string SomePrinterFunction() {
  std::string output;
  google::protobuf::io::StringOutputStream output_stream(&output);
  google::protobuf::io::Printer printer(&output_stream, '$');

  printer.Print("Foo");

  return output;
}

Then depending on your compiler, it may decide to call the move operator of std::string on that return statement. At that point, output is becoming empty, and its size becomes zero. Meaning that this assert here will fire, during the actual destruction of printer:

https://github.com/google/protobuf/blob/master/src/google/protobuf/io/zero_copy_stream_impl_lite.cc#L190

This is currently causing this issue on grpc: grpc/grpc#1769.

I would also argue that the right thing to do would be to avoid calling Shrink altogether in the destructor of Printer, and let the user do that manually, but I didn't want to change actual code behavior at that point.

…tput stream will disappear before our destructor.
@xfxyjwf
Copy link
Contributor

xfxyjwf commented May 28, 2015

Although it might not be obvious, the correct way to use StringOutputStream is to wrap it in an inner block to make sure the stream is destructed before using the output string. For example:

std::string SomePrinterFunction() {
  std::string output;
  {
    google::protobuf::io::StringOutputStream output_stream(&output);
    google::protobuf::io::Printer printer(&output_stream, '$');
    printer.Print("Foo");
  }
  return output;
}

Basically before using the output string, users must ensure all data has been written through all piped output streams and as ZeroCopyOutputStream doesn't have a Flush() method, the only way to flush the stream is to destruct it. Therefore we use code structs like the above quite a lot in protobuf code base.

For this issue in grpc, updating grpc code to wrap the stream in an inner block might be a better solution.

@nicolasnoble
Copy link
Contributor Author

Well, I'd argue that this is a very counter intuitive API, which is very error prone and difficult to understand and debug. In fact, it took us a while to understand what was going on.

But fair enough, I can think of other issues with that pull request anyway. We'll do that for now in gRPC, and I'll come up with a better patch for protobuf later on, that alleviates the problem for any future user.

bithium pushed a commit to bithium/protobuf that referenced this pull request Sep 4, 2023
Added util library for converting defs back to descriptor protos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants