apache / apache/brpc

UBRing transport: timer lifecycle, backend-specific chunking, and maintainability issues

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

Description

**Describe the bug**

After reviewing and testing the UBRing/UBShmTransport introduced by #3290, we found several correctness, scalability, portability, performance, and maintainability issues.

The two shared-memory backends should be considered separately:

- `--ub_shm_type=1`: local POSIX shared memory (IPC)
- `--ub_shm_type=2`: remote shared memory provided by UBS

Some constraints that may be necessary for UBS do not apply to local IPC shared memory.

### 1. Deleting a timer may block indefinitely

Linux timerfds are created as blocking file descriptors:

```cpp
timerfd_create(CLOCK_MONOTONIC, 0);
```

Both `DeleteTimerInner()` and `DeleteTimerSafe()` execute a blocking read before closing the fd:

```cpp
uint64_t exp = 0;
read(fd, &exp, sizeof(exp));
close(fd);
```

There is a deterministic problematic path for one-shot timers:

1. `TimerEpoll()` reads and consumes the timerfd expiration.
2. It calls `UnifiedCallback()`.
3. For a non-periodic timer, `UnifiedCallback()` calls `DeleteTimerInner()`.
4. `DeleteTimerInner()` reads the same timerfd again.
5. Since the one-shot timer has already expired and was drained, the second blocking `read()` may never return.

For periodic timers, deletion may also block until the next expiration. For example, deleting a heartbeat timer may block for up to the heartbeat interval.

This can stall connection cleanup and delay releasing resources after a peer disconnects or crashes.

Timer deletion should not require a blocking read. If the custom implementation is retained, timerfds should at least use `TFD_NONBLOCK | TFD_CLOEXEC`, and `EAGAIN` should be treated as normal. Removing the fd from epoll and closing it without draining it may be sufficient.

### 2. The custom timer subsystem should preferably be replaced with brpc's existing timer implementation

The timer problems are not isolated defects. They are consequences of introducing a second timer framework based on timerfd/epoll alongside the timer framework already maintained by brpc.

brpc already provides:

- `bthread::TimerThread`
- `bthread_timer_add()` and `bthread_timer_del()`
- Versioned timer task IDs
- Cancellation semantics that account for a callback already running
- A shared global timer thread

UBRing can schedule one-shot tasks through the global `bthread::TimerThread`. Periodic heartbeat or connection-check tasks can reschedule themselves after each callback. Potentially blocking resource cleanup should be handed off to a bthread or worker; the timer callback should only validate state and schedule the cleanup work.

Unless UBRing has a concrete timer requirement that the existing implementation cannot satisfy, maintaining an independent timer subsystem does not appear justified. Reusing the existing implementation would eliminate most of the following newly introduced problems.

#### 2.1 Timer context storage scales with `RLIMIT_NOFILE`

`TimerInit()` allocates a context for every possible process fd and initializes a spinlock for every entry:

```cpp
getrlimit(RLIMIT_NOFILE, &rlim);
g_max_system_fd = (uint32_t)rlim.rlim_cur;
g_timer_fd_ctx_map =
(TimerFdCtx*)malloc(sizeof(TimerFdCtx) * g_max_system_fd);
```

On x86-64, `sizeof(TimerFdCtx)` is approximately 32 bytes:

| `RLIMIT_NOFILE` | Approximate timer table size |
|---:|---:|
| 1,024 | 32 KiB |
| 20,480,000 | 625 MiB |

The allocation and initialization cost depends on the maximum possible fd count rather than the number of active UBRing timers.

#### 2.2 Using the numeric fd as timer identity introduces an fd-reuse race

Epoll stores only the numeric fd:

```cpp
event.data.fd = timer_fd;
```

The callback context is found through:

```cpp
g_timer_fd_ctx_map[timer_fd]
```

After a timer is removed and closed, Linux may immediately reuse the fd for another timer. A stale epoll event containing the old fd can then refer to the new timer context.

Possible consequences include executing a new timer's callback for an old event, deleting the wrong timer, or accessing callback arguments after their owning connection has been released.

Changing the container from an fd-indexed array to a map alone is not sufficient. Stale events need to be rejected with a generation-safe identity. `bthread::TimerThread` already uses versioned task IDs for this purpose.

#### 2.3 Timer initialization contains a race

`StartTimeEpoll()` creates the timer thread before `TimerInit()` sets:

```cpp
g_timer_module_initialized = 1;
```

The new thread may run immediately and observe the old value:

```cpp
if (g_timer_module_initialized <= 0) {
break;
}
```

In that case, the timer thread exits during initialization. Initialization and destruction also use non-atomic global state without an explicit initialization lock.

#### 2.4 The default close-check timer is too frequent

The close-check timer is configured as:

```cpp
DEFINE_int32(ub_event_queue_timer_interval, 100, ...);

uint32_t interval = FLAGS_ub_event_queue_timer_interval * 1000;
```

The resulting default period is 100 microseconds, or 10,000 wakeups per second for every UBRing connection. Each connection also creates heartbeat and cleanup timers.

The close transition should preferably be event-driven. If polling is unavoidable, the default interval should be significantly coarser and its unit should be documented.

#### 2.5 All timer callbacks execute serially on one pthread

`TimerEpoll()` invokes callbacks inline from the only timer thread. Some callbacks perform connection cleanup and may wait for `ub_flying_io_timeout`, currently five seconds.

A slow callback therefore delays heartbeat and cleanup callbacks for all other UBRing connections. Timer callbacks should schedule potentially blocking work on an appropriate bthread or worker instead of executing it inline.

### 3. The macOS timerfd emulation is not functionally equivalent

The macOS implementation creates a pipe and returns only its read end:

```cpp
static int timerfd_create_macosx(...) {
int pipefd[2];
pipe(pipefd);
return pipefd[0];
}
```

There are several issues:

- The pipe write end is leaked.
- Nothing writes expiration counters into the pipe.
- `TimerEpoll()` calls `read(timer_fd, ...)` after receiving an `EVFILT_TIMER` event, so it can block forever on the empty pipe.
- `timerfd_settime_macosx()` is a no-op.
- `EV_SET()` uses `it_value` as the repeating kqueue interval and does not correctly model `it_interval`.
- Many timers use an initial expiration of one nanosecond, which becomes zero milliseconds after conversion.

The kqueue path should process the expiration count from `struct kevent::data` and should not read from a pipe. Reusing brpc's existing timer implementation would remove the need for this platform-specific emulation.

### 4. The fixed 60-byte payload should be handled differently for IPC and UBS

The current message format is fixed to one 64-byte cache line:

```cpp
#define UBR_MSG_HEADER_LEN 4
#define UBR_MSG_PAYLOAD_LEN 60
#define UBR_MSG_LEN 64
```

The current send path first assembles the packet in local memory:

```cpp
memcpy(local_msg_space.payload, input, length);
Copy64Byte(remote_data_queue_slot, &local_msg_space);
```

This design should not be applied identically to both backends.

#### 4.1 Local IPC shared memory

For the IPC backend, both processes map the same POSIX shared-memory object. The sender can write directly into the destination ring slot.

The current staging path introduces an avoidable copy:

```text
IOBuf/iovec
-> local_msg_space
-> shared-memory ring slot
```

It can instead be:

```text
IOBuf/iovec
-> shared-memory ring slot payload
-> publish the slot state
```

The producer should write the payload and metadata first, then publish the slot with release semantics. The consumer should check the slot state with acquire semantics before reading the payload.

This removes one local memory copy and allows the IPC backend to use a more suitable payload size. A 4 KiB or 64 KiB attachment currently has to be split into approximately 69 or 1,093 chunks respectively.

The IPC payload size could be a startup-only option, with the selected format included in the handshake. It must not be changed while connections using that format are alive.

#### 4.2 Remote UBS shared memory

For UBS, the 64-byte unit may be required by the UB transport's indivisible remote-store semantics. If that hardware/API requirement is confirmed, UBS may need to continue assembling a complete 64-byte packet locally before publishing it remotely.

However, this is a backend-specific constraint and should not force the IPC backend to use the same format.

The non-`LS64` implementation of `Copy64Byte()` currently falls back to ordinary `memcpy()`:

```cpp
#ifdef LS64
// ST64B
#else
memcpy(dst, src, 64);
#endif
```

An ordinary `memcpy()` does not itself guarantee an indivisible 64-byte store. The required atomicity and visibility guarantees should therefore be documented and validated separately for ARM LS64/ST64B, other local architectures, UBS remote mappings, and POSIX IPC shared memory.

A possible design is to let each backend provide its own payload size, slot alignment, publication operation, and memory-ordering requirements. The chosen format or format version should be negotiated during the UBRing handshake.

### 5. There are several code-style and naming issues

The change contains personal debugging remnants, ambiguous flag names, and terminology copied from the RDMA transport without adapting it to UBRing.

#### 5.1 Personal debugging remnants should be removed

`src/brpc/ubshm_transport.cpp` contains commented debugging logs with a personal identifier:

```cpp
// LOG(INFO) << "mwj pollin4=" << pollin ...
// LOG(INFO) << "mwj pollin1=" << pollin;
// LOG(INFO) << "mwj pollin2=" << pollin << " mwj_ret=" << mwj_ret;
// LOG(INFO) << "mwj return 0";
```

It also contains:

```cpp
auto mwj_ret = bthread::butex_wait(...);
```

`src/brpc/ubshm/ub_endpoint.cpp` contains:

```cpp
// TODO mwj should polling start after the connection is established?
```

Personal debugging logs should be deleted rather than committed as comments. The local variable should have a descriptive name such as `wait_rc`. The TODO should either be resolved or rewritten as a clear actionable comment without an author identifier.

#### 5.2 Time-related gflags do not document their units

The following flags do not indicate their units in either the flag name or description:

```cpp
DEFINE_int32(ub_disconnect_timeout, 5, "Ubshm disconnection timeout.");
DEFINE_int32(ub_connect_timeout, 1, "Ubshm connection timeout.");
DEFINE_int32(ub_hb_timer_interval, 5, "Heartbeat timer interval.");
DEFINE_int32(ub_event_queue_timer_interval, 100,
"Interval of the disconnection timer.");
DEFINE_int32(ub_flying_io_timeout, 5,
"Waiting time for stopping data sending and receiving...");
```

The code uses different units:

- `ub_disconnect_timeout`: seconds
- `ub_connect_timeout`: seconds
- `ub_hb_timer_interval`: seconds
- `ub_event_queue_timer_interval`: microseconds
- `ub_flying_io_timeout`: seconds

A user cannot determine these units from `--help`. The unit should be encoded in the flag name and description, for example:

```text
ub_disconnect_timeout_s
ub_connect_timeout_s
ub_hb_timer_interval_s
ub_event_queue_timer_interval_us
ub_flying_io_timeout_s
```

If renaming is considered incompatible, the descriptions must at least state the exact units.

The conversion should also use an existing named constant or time utility instead of:

```cpp
#define TIME_COVERSION 1000
```

`TIME_COVERSION` is misspelled and does not explain what is being converted. The existing `USEC_TO_NSEC` constant or a standard time conversion helper would be clearer.

#### 5.3 `_cq_sid` is RDMA terminology and is inappropriate for UBRing

`UBShmEndpoint` uses the following names:

```cpp
SocketId _cq_sid;
struct CqSidOp;
std::unordered_set cq_sids;
```

It also logs:

```cpp
PLOG(WARNING) << "Fail to create socket for cq";
```

These names appear to have been copied from `RdmaEndpoint`. In RDMA, `CQ` means Completion Queue, so `_cq_sid` describes a socket associated with an RDMA completion queue. UBRing does not have an RDMA completion queue.

In `UBShmEndpoint`, this synthetic `SocketId` represents an entry registered with the UBRing poller. Names should describe that role, for example `_poller_sid`, `PollerSidOp`, and `poller_sids`, or equivalent event-source terminology.

Copying the RDMA polling structure may be reasonable, but RDMA-specific terminology should not be retained when the underlying concept is different.

### 6. Current tests do not cover lifecycle and data-path cases

`test/brpc_ubring_unittest.cpp` currently covers hello-message serialization, endpoint construction, basic IPC allocation, and reset behavior.

It does not cover:

- End-to-end UBRing send and receive
- Large attachments spanning many chunks
- Client or server crash/disconnect cleanup
- One-shot timer expiration and deletion
- Concurrent timer deletion and callback execution
- Numeric fd reuse with stale epoll events
- High `RLIMIT_NOFILE`
- Multiple concurrent UBRing connections
- macOS timer behavior
- Different IPC/UBS message formats
- Validation of time-related gflag units and default intervals

### 7. Bazel and CI support was incomplete, but has been fixed

The original UBRing change documented:

```bash
bazel build --define=with_ubring=true
```

but the Bazel build did not define `BRPC_WITH_UBRING`, did not expose the performance example targets, and the Bazel CI jobs did not build or run UBRing-enabled unit tests.

This has already been fixed by #3445, which added:

- `--define=BRPC_WITH_UBRING=true`
- `--config=ubring`
- Bazel targets for the UBRing performance client and server
- UBRing-enabled Bazel compilation and unit-test jobs
- Updated English and Chinese documentation

No further action is requested for this resolved build issue. It is listed here for completeness because it prevented the original implementation from being exercised by Bazel CI.

**To Reproduce**

#### Cleanup hang

Build brpc and the performance example:

```bash
bazel build --config=ubring \
//example:ubring_performance_server \
//example:ubring_performance_client
```

Start the IPC server:

```bash
./bazel-bin/example/ubring_performance_server \
--use_ubring=true \
--ub_shm_type=1 \
--port=8002
```

Run a client and let it exit or terminate it:

```bash
./bazel-bin/example/ubring_performance_client \
--use_ubring=true \
--ub_shm_type=1 \
--servers=127.0.0.1:8002 \
--thread_num=1 \
--queue_depth=1 \
--attachment_size=4096 \
--test_seconds=30
```

Inspect a process that becomes stuck during cleanup. The stack can stop in the blocking `read()` called by `DeleteTimerInner()` or `DeleteTimerSafe()`.

#### Timer table allocation

```bash
ulimit -n 20480000
```

Initialize the UBRing timer module and inspect startup time and RSS. `TimerInit()` allocates and initializes approximately 625 MiB of timer contexts even when no UBRing connection exists.

#### IPC chunking

Send 4 KiB and 64 KiB attachments through the IPC backend. The current 60-byte payload creates approximately:

```text
4 KiB -> 69 chunks
64 KiB -> 1093 chunks
```

**Expected behavior**

- UBRing should reuse brpc's existing timer framework unless a concrete unsupported requirement is documented.
- Timer deletion must never block waiting for another expiration.
- Timer callbacks and deletion must be safe when executed concurrently.
- Stale epoll events must not operate on a new timer that reused the same fd.
- Timer bookkeeping should scale with active timers, not `RLIMIT_NOFILE`.
- Timer callbacks should perform lightweight state transitions and dispatch blocking cleanup elsewhere.
- macOS timers should have behavior equivalent to the Linux implementation.
- IPC and UBS should use backend-appropriate payload and publication mechanisms.
- IPC should write directly into shared-memory slots and publish them with explicit release/acquire ordering.
- UBS should retain a 64-byte packet only if required by documented hardware/API semantics.
- The active message format must be agreed by both peers.
- All time-related gflags should include their units in their names and descriptions.
- UBRing code should not contain personal debugging markers or RDMA-specific terminology for unrelated concepts.
- CI tests should exercise the full send/receive and connection lifecycle.

**Versions**

OS: Helix 8.4r, x86-64
Compiler: GCC 8.5.0
brpc: UBRing introduced by `72bf13a3` (#3290); issues still present on current master
protobuf: Not relevant to these issues
Build: Bazel UBRing support fixed by #3445

**Additional context/screenshots**

We performed local IPC performance tests with 32 MiB data queues. An experimental version used an 8 KiB payload and wrote directly into the IPC ring slot.

| Workload | Original 60B format | IPC 8 KiB direct-write |
|---|---:|---:|
| 4 threads, depth 64, 4 KiB attachment | 761 MB/s | 871-905 MB/s |
| 4 threads, depth 64, 64 KiB attachment | 1,869 MB/s | 3,559-3,925 MB/s |
| 1 thread, depth 1, 4 KiB attachment | 102.6 MB/s | 106.4-107.4 MB/s |

These numbers are directional rather than a strict single-variable A/B comparison because both the chunk payload size and the IPC copy path were changed. They nevertheless show that the fixed 60-byte format has a significant cost for larger messages.

The experimental direct-write IPC path was:

```text
IOBuf/iovec
-> destination shared-memory payload
-> write metadata
-> release-store the ready flag
```

The receiver used an acquire-load of the ready flag before consuming the payload.

Contributor guide

Open the contributing guide

Research direction

Start by reading src/brpc/ubshm_transport.cpp and src/brpc/ubshm/ub_endpoint.cpp, then trace TimerInit(), StartTimeEpoll(), TimerEpoll(), and the timer deletion paths. Review how Copy64Byte() and the two shared-memory backends handle payloads and publication. Done means the timer lifecycle and macOS path are safe, backend-specific chunking is defined, and the listed naming and flag issues are resolved.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
distributed-systems, networking
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.