Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR updates the AMD stats server shutdown path to silence a g++ “unused result” warning when signaling the background thread to exit.
Changes:
- Capture the return value of
write()inStatsServer::~StatsServer()using a[[maybe_unused]]variable to quiet compiler warnings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint64_t i{1}; | ||
| [[maybe_unused]] ssize_t nbytes; | ||
|
|
||
| nbytes = write(m_efd.get(), &i, sizeof(i)); | ||
| m_thread.join(); |
There was a problem hiding this comment.
write() is used to wake the polling thread so the destructor can join(). If this write() fails (e.g., returns -1 on EINTR), the polling thread may never be notified and the destructor can block indefinitely in join(). Since you now capture the return value, it’d be safer to retry on EINTR and handle hard failures (e.g., by also making threadFn() break on POLLERR/POLLNVAL for the eventfd, or otherwise ensuring the thread is guaranteed to exit).
There was a problem hiding this comment.
@zbyrne Is this valid? Does this need a real fix?
There was a problem hiding this comment.
yeah, sounds valid. Shame we aren't using C++20 with jthreads and stop_tokens
There was a problem hiding this comment.
Okay, I'm going to close this PR then while @zbyrne implements a real fix
Fix suggested by copilot in #233
No description provided.