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

[ZhengweiZhu]

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:

timerfd_create(CLOCK_MONOTONIC, 0);

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

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:

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:

event.data.fd = timer_fd;

The callback context is found through:

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:

g_timer_module_initialized = 1;

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

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:

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:

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:

#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:

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:

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

It can instead be:

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():

#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:

// 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:

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

src/brpc/ubshm/ub_endpoint.cpp contains:

// 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:

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:

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:

#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:

SocketId _cq_sid;
struct CqSidOp;
std::unordered_set<CqSidOp, ...> cq_sids;

It also logs:

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:

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:

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

Start the IPC server:

./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:

./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

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:

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:

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.

[zchuango]

Thanks for the detailed review and testing.

Since the UBRing/UBShmTransport was originally contributed in #3290, we'd like to address these findings as part of a broader UBShmTransport improvement effort, with the changes split into focused PRs to make the improvements easier to review and track.

After an initial review, we plan to organize the follow-up work into the following phases. The exact PR boundaries may be adjusted as we validate the implementation details and receive feedback from the community.

Phase 1: Code quality and maintainability
We will first address low-risk maintainability issues, including:

• removing debugging leftovers;
• improving unclear naming and RDMA-specific terminology in UBRing code;
• clarifying time-related configuration units and comments.

Phase 2: Timer and lifecycle stability
We will prioritize the timer-related correctness and lifecycle issues.

This phase will cover:

• reproducing and fixing timer deletion and cleanup issues;
• reviewing callback/deletion lifecycle races;
• improving stale fd / fd reuse safety;
• avoiding potentially blocking cleanup paths.

Phase 3: Backend abstraction and data-path improvements
We will further review the differences between IPC and UBS backends, especially around:

• message layout and payload size;
• publication semantics and memory ordering;
• backend-specific optimization opportunities.

Phase 4: Test coverage and continuous hardening
Regression and end-to-end tests will be added together with each improvement, covering areas such as:

• timer lifecycle;
• disconnect cleanup;
• large attachments;
• multiple concurrent connections;
• backend-specific data paths.

@ZhengweiZhu @wwbmmm @chenBright welcome further feedback and suggestions from the community. Please feel free to ping me if there are any concerns or if you would like to discuss any of the proposed directions. We will keep this issue updated as the work progresses.