FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix abort when binding response streams to a failed socket by amoxic · Pull Request #3250 · apache/brpc · GitHub

/ brpc Public

Fix abort when binding response streams to a failed socket - #3250

Open
amoxic wants to merge 1 commit into
apache:masterfrom
amoxic:fix-3245-stream-bind-failure
ConversationCommits (1)ChecksFiles changed
Open

Fix abort when binding response streams to a failed socket#3250
amoxic wants to merge 1 commit into
apache:masterfrom
amoxic:fix-3245-stream-bind-failure

Conversation

amoxic commented Mar 25, 2026

Copy link
Copy Markdown

What problem does this PR solve?

Issue Number: resolves #3245

Problem Summary:

When a peer disconnects after StreamAccept() succeeds but before the RPC response binds the response stream to the host socket, Socket::AddStream() can fail inside Stream::SetHostSocket(). The previous code treated this as an impossible state and aborted the server with CHECK(false).

This PR turns that path into a normal runtime failure so the current RPC/stream is failed and the server process stays alive.

What is changed and the side effects?

Changed:

  • Make Stream::SetHostSocket() return -1 with errno instead of aborting when Socket::AddStream() fails.
  • Handle response stream bind failures in SendRpcResponse() by setting controller failure, failing the affected stream(s), and returning early.
  • Keep response write completion joined through a scope guard so tracing data is still finalized on early returns.
  • Add unit tests for SetHostSocket() failure and the server-side disconnect-before-response path.

Side effects:

  • Performance effects:

    No expected steady-state performance impact. Extra work is only added on the error path.

  • Breaking backward compatibility:

    No API change. The behavior changes from process abort to RPC/stream failure when the host socket is already failed.

Check List:

hjwsm1989 commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

@chenBright please help review

chenBright requested a review from Copilot March 25, 2026 11:53

chenBright commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

cc @jenrryyou

Copilot AI reviewed Mar 25, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull request overview

This PR prevents the server process from aborting when binding response streams to a host socket that has already failed (e.g., peer disconnects between StreamAccept() and response send), turning it into a normal RPC/stream failure path.

Changes:

  • Make Stream::SetHostSocket() return an error (with errno set) instead of CHECK(false) on Socket::AddStream() failure.
  • In SendRpcResponse(), handle response-stream bind failures by failing the controller and affected stream(s), and returning early while still joining background response-write (via a scope guard).
  • Add unit tests covering SetHostSocket() failure and the disconnect-before-response scenario.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
src/brpc/stream.cpp Converts host-socket bind from fatal abort to error return.
src/brpc/policy/baidu_rpc_protocol.cpp Handles stream bind failures in SendRpcResponse() and ensures response-write completion is joined on early returns.
test/brpc_streaming_rpc_unittest.cpp Adds regression tests for failed-socket bind and server-side early disconnect.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/brpc/policy/baidu_rpc_protocol.cpp
Comment thread src/brpc/stream.cpp Outdated
Comment thread src/brpc/stream.cpp
Comment thread src/brpc/policy/baidu_rpc_protocol.cpp
Comment thread src/brpc/policy/baidu_rpc_protocol.cpp
amoxic force-pushed the fix-3245-stream-bind-failure branch from 88a7a19 to b567af3 Compare March 25, 2026 16:32

amoxic commented Mar 26, 2026

Copy link
Copy Markdown
Author

Thanks for the review. Addressed all Copilot comments:

  1. Stream::SetHostSocket now records the first failure in _set_host_socket_ec and always sets errno from that value on failure (no more call_once stack-local loss).
  2. All callers now check SetHostSocket return and fail controller/streams on error: SendRpcResponse main stream + extra streams, Controller::HandleStreamConnection.
  3. Bind failures propagate errno directly (no reliance on sock->non_zero_error_code).
  4. Stream::SetFailed now preformats the reason string (SetFailedWithReason helper) to avoid varargs UB; all new call sites use the safe path.
  5. Added regression tests covering SetHostSocket failure and server-side disconnect-before-response; both pass locally (brpc_streaming_rpc_unittest filtered run).

amoxic force-pushed the fix-3245-stream-bind-failure branch from b567af3 to 83e746b Compare March 27, 2026 12:30

amoxic commented Mar 30, 2026

Copy link
Copy Markdown
Author

@chenBright could you please review this pr again

chenBright reviewed May 8, 2026
View reviewed changes
Comment thread src/brpc/policy/baidu_rpc_protocol.cpp
Stream::SetFailed(response_stream_ids, errcode,
"Fail to bind response stream to %s",
sock->description().c_str());
return;

chenBright May 8, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think that if binding the response stream fails, we shouldn't just return directly; we should still try writing a response.

Comment thread src/brpc/policy/baidu_rpc_protocol.cpp
Comment on lines +456 to +457
cntl->SetFailed(errcode, "Fail to bind response stream to %s",
sock->description().c_str());

chenBright May 8, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just call cntl->SetFailed once.

Comment thread src/brpc/policy/baidu_rpc_protocol.cpp
s->FillSettings(stream_settings);
s->SetHostSocket(sock);
if (s->SetHostSocket(sock) != 0) {
const int errcode = errno;

chenBright May 8, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why use errno as the reason for SetHostSocket failure? SetHostSocket failure does not set errno.

chenBright commented May 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

@chenBright could you please review this pr again

@amoxic Sorry, I missed the message. I've already reviewed the PR.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

chenBright chenBright left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] 对端在 StreamAccept() 之后、响应发送之前断开时，Stream::SetHostSocket() 会触发 CHECK(false) 导致进程崩溃

4 participants

Back | FazBrowse Home | New Git URL