Copilot commented on code in PR #12925:
URL: https://github.com/apache/trafficserver/pull/12925#discussion_r3552693661
##########
src/iocore/net/UnixNetAccept.cc:
##########
@@ -387,7 +387,7 @@ NetAccept::do_blocking_accept(EThread *t)
case -1:
[[fallthrough]];
default:
- if (!action_->cancelled) {
+ if (action_->server.load(std::memory_order_acquire) != nullptr &&
!action_->cancelled) {
Review Comment:
This reads `action_->cancelled` without synchronization, but
`Action::cancelled` is a non-atomic `bool`. Since `cancel()` first
`exchange()`s `server` to `nullptr`, checking `server` is sufficient to
suppress the post-cancel EBADF path without introducing a data race.
##########
src/iocore/net/UnixNetAccept.cc:
##########
@@ -112,7 +112,7 @@ net_accept(NetAccept *na, void *ep, bool blockable)
if (res == -EAGAIN || res == -ECONNABORTED || res == -EPIPE) {
goto Ldone;
}
- if (na->server.sock.is_ok() && !na->action_->cancelled) {
+ if (na->action_->server.load(std::memory_order_acquire) != nullptr &&
!na->action_->cancelled) {
Review Comment:
`Action::cancelled` is a plain `bool` (not atomic) and is accessed here
without a lock, so this check can create a data race/UB. The new `server`
atomic pointer already indicates cancellation/closure, so you can drop the
`!cancelled` read here.
##########
src/iocore/net/UnixNetAccept.cc:
##########
@@ -580,7 +580,7 @@ NetAccept::acceptFastEvent(int event, void *ep)
check_transient_accept_error(res);
goto Ldone;
}
- if (!action_->cancelled) {
+ if (action_->server.load(std::memory_order_acquire) != nullptr &&
!action_->cancelled) {
Review Comment:
`action_->cancelled` is not atomic, so checking it here (outside any lock)
is a data race. The `server` atomic pointer is already the concurrency-safe
indicator that accept should still report errors, so remove the `!cancelled`
part of the condition.
--
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]
