rajvarun77 commented on code in PR #3330:
URL: https://github.com/apache/brpc/pull/3330#discussion_r3384327675
##########
src/brpc/socket.cpp:
##########
@@ -578,6 +579,9 @@ int Socket::ResetFileDescriptor(int fd) {
_avg_msg_size = 0;
// MUST store `_fd' before adding itself into epoll device to avoid
// race conditions with the callback function inside epoll
+ static butil::atomic<uint64_t> BAIDU_CACHELINE_ALIGNMENT fd_version(0);
+ _fd_version.store(fd_version.fetch_add(1, butil::memory_order_relaxed),
Review Comment:
Thanks — you're right that the *uniqueness* of the value comes from the
static `fd_version` atomic. But the per-`Socket` `_fd_version` still needs to
be atomic for a different reason: it is written and read from different threads
with no lock between them.
- Writer: `ResetFileDescriptor()` stores `_fd_version` on the reconnect path
(`CheckConnectedAndKeepWrite` -> `ResetFileDescriptor`), with no socket mutex
held.
- Reader: the mysql prepared-statement path reads `socket->fd_version()` on
the RPC bthread (`MysqlStatement::StatementId`). It obtains the socket via
`Socket::Address()`, which only gates on the `SocketId` version — and
`ResetFileDescriptor` does not bump that version — so `Address()` succeeds even
while the fd is being reset. The read can therefore overlap the write on the
same Socket.
A concurrent non-atomic read/write on the same object is a data race (UB),
and a 64-bit load is not tear-free on 32-bit targets, so the member has to be
atomic. This mirrors `_fd` itself, which is a `butil::atomic<int>` written in
the same `ResetFileDescriptor` and read lock-free (`memory_order_relaxed`) all
over the IO path. I kept `_fd_version` on the same relaxed-atomic pattern.
Happy to add a brief comment to that effect if it helps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
