apache / apache/brpc

Sometimes SetFailed() is called with errno=0 in InputMessenger::OnNewMessages()

Open
#1,860 11 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C++
Stars
17.6k
Forks
4.1k
Avg merge
2d 12h
Merged PRs (30d)
69

Description

**Describe the bug (描述bug)**
[`m->SetFailed()`](https://github.com/apache/incubator-brpc/blob/1.1.0/src/brpc/input_messenger.cpp#L227) is called with `saved_errno == 0` in [`InputMessenger::OnNewMessages()`](https://github.com/apache/incubator-brpc/blob/1.1.0/src/brpc/input_messenger.cpp#L180), which then triggers a hard error at [`CHECK(false) << "error_code is 0"`](https://github.com/apache/incubator-brpc/blob/1.1.0/src/brpc/socket.cpp#L809).

**To Reproduce (复现方法)**
In our environment we can constantly reproduce this issue in ___LTO (`-flto=full`)___ and ___ThinLTO (`-flto=thin`)___ modes, but ___everything runs without error in normal (`-fno-lto`) mode___.

I surfed a bit into the source code, and it turns out that

- [`readv()`](https://github.com/apache/incubator-brpc/blob/1.1.0/src/butil/iobuf.cpp#L1585) may return `-1` with `errno == 0` in [`pappend_from_file_descriptor()`](https://github.com/apache/incubator-brpc/blob/1.1.0/src/butil/iobuf.cpp#L1551), which then cause
- [`m->DoRead()`](https://github.com/apache/incubator-brpc/blob/1.1.0/src/brpc/input_messenger.cpp#L213) returns `nr == -1` with `errno == 0` in [`InputMessenger::OnNewMessages()`](https://github.com/apache/incubator-brpc/blob/1.1.0/src/brpc/input_messenger.cpp#L180), which then run into
- [`else if (errno != EAGAIN)`](https://github.com/apache/incubator-brpc/blob/1.1.0/src/brpc/input_messenger.cpp#L221) branch with `errno == 0` and triggers the `CHECK` failure.

By adding several pieces of logging codes _with cares taken not to unintentionally change_ `errno`, I can confirm the behavior described above, see the attached screenshot for reference.

A potential fix / workaround is to change [`if (errno == EINTR)`](https://github.com/apache/incubator-brpc/blob/1.1.0/src/brpc/input_messenger.cpp#L222) to `if (error == 0 || errno == EINTR)`, but I'm not familiar with BRPC code (nor did I figure out why link-time optimization triggers such an issue), so I'd prefer BRPC maintainers taking a look at it. Thanks!

**Expected behavior (期望行为)**
Consistent behavior regardless of link-time optimization options.

**Versions (各种版本)**
OS: Linux Ubuntu 18.04 in Docker
Compiler: Clang 12.0.1
brpc: 1.1.0 Release
protobuf: 21.1

**Additional context/screenshots (更多上下文/截图)**

20220727045020

Contributor guide

Open the contributing guide

Research direction

Start with InputMessenger::OnNewMessages() and follow DoRead() into butil/iobuf.cpp's pappend_from_file_descriptor(), then inspect the SetFailed() path and CHECK in socket.cpp. Reproduce with Clang 12 on Ubuntu 18.04 under full or ThinLTO and compare normal builds. Done means the LTO configurations no longer trigger a hard failure from an errno value of 0 and behavior is consistent across modes.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, linux
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.