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

Check append return code #1762

Merged
merged 1 commit into from
Jun 20, 2022
Merged

Check append return code #1762

merged 1 commit into from
Jun 20, 2022

Conversation

wwbmmm
Copy link
Contributor

@wwbmmm wwbmmm commented May 13, 2022

Fix #1761

@@ -80,11 +80,11 @@ static void SerializeRpcHeaderAndMeta(
::google::protobuf::io::CodedOutputStream coded_out(&arr_out);
meta.SerializeWithCachedSizes(&coded_out); // not calling ByteSize again
CHECK(!coded_out.HadError());
out->append(header_and_meta, sizeof(header_and_meta));
CHECK_EQ(0, out->append(header_and_meta, sizeof(header_and_meta)));
Copy link

@amosbird amosbird May 13, 2022

Choose a reason for hiding this comment

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

IIUC this is just an assertion and will only be effective in debug build. Perhaps we should change the return type of SerializeRpcHeaderAndMeta into bool and handle it properly, similar to what we did for SerializeAsCompressedData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// The CHECK(condition) macro is active in both debug and release builds and
// effectively performs a LOG(FATAL) which terminates the process and
// generates a crashdump unless a debugger is attached.

https://github.com/apache/incubator-brpc/blob/master/src/butil/logging.h#L122

Choose a reason for hiding this comment

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

Ok... It's interesting to see a release assertion macro) However, this error is recoverable. Resorting to std::terminate seems to be an overkill.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@zyearn zyearn merged commit a4c4e1a into apache:master Jun 20, 2022
@wwbmmm wwbmmm deleted the check_append branch August 31, 2022 02:26
@Huixxi Huixxi added the bug the code does not work as expected label Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug the code does not work as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What's the behavior of SendRpcResponse in memory constrained env?
5 participants