diff --git a/src/iocore/net/P_NetAccept.h b/src/iocore/net/P_NetAccept.h index 89650e34fa6..b3ca8e654ee 100644 --- a/src/iocore/net/P_NetAccept.h +++ b/src/iocore/net/P_NetAccept.h @@ -62,23 +62,38 @@ AcceptFunction net_accept; class UnixNetVConnection; struct NetAcceptAction : public Action, public RefCountObjInHeap { - std::atomic server{nullptr}; - NetAcceptAction(Continuation *cont, Server *s) { continuation = cont; if (cont != nullptr) { mutex = cont->mutex; } - server.store(s, std::memory_order_release); + _server.store(s, std::memory_order_release); + } + + /** Whether this action still owns an open listening socket. + + Accept paths must consult this before dispatching EVENT_ERROR. It is + cleared by cancel() before Action::cancel() sets @c cancelled, so it goes + false no later than the cancellation the continuation is aware of, and it + is safe to read from any thread. Reading @c cancelled instead is both a + data race on a plain bool and too late: accept() reports EBADF as soon as + the socket closes, which is before @c cancelled is set. + + */ + bool + is_listening() const + { + return _server.load(std::memory_order_acquire) != nullptr; } void cancel(Continuation *cont = nullptr) override { // Use atomic exchange so only one thread closes the server, preventing - // use-after-free races between cancel() and acceptEvent() cleanup. - Server *s = server.exchange(nullptr, std::memory_order_acq_rel); + // use-after-free races between cancel() and acceptEvent() cleanup. This + // must stay ahead of Action::cancel(), see is_listening(). + Server *s = _server.exchange(nullptr, std::memory_order_acq_rel); if (s != nullptr) { s->close(); } @@ -92,6 +107,11 @@ struct NetAcceptAction : public Action, public RefCountObjInHeap { static DbgCtl dbg_ctl{"net_accept"}; Dbg(dbg_ctl, "NetAcceptAction dying"); } + +private: + /// Cleared exactly once, by cancel(). Private so the ordering above is the + /// only way this transitions to null. + std::atomic _server{nullptr}; }; // diff --git a/src/iocore/net/UnixNetAccept.cc b/src/iocore/net/UnixNetAccept.cc index 66e73096fa2..9046dbaf086 100644 --- a/src/iocore/net/UnixNetAccept.cc +++ b/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_->is_listening()) { if (!blockable) { na->action_->continuation->handleEvent(EVENT_ERROR, reinterpret_cast(res)); } else { @@ -387,7 +387,7 @@ NetAccept::do_blocking_accept(EThread *t) case -1: [[fallthrough]]; default: - if (!action_->cancelled) { + if (action_->is_listening()) { SCOPED_MUTEX_LOCK(lock, action_->mutex ? action_->mutex : t->mutex, t); action_->continuation->handleEvent(EVENT_ERROR, reinterpret_cast(res)); Warning("accept thread received fatal error: errno = %d", errno); @@ -580,7 +580,7 @@ NetAccept::acceptFastEvent(int event, void *ep) check_transient_accept_error(res); goto Ldone; } - if (!action_->cancelled) { + if (action_->is_listening()) { action_->continuation->handleEvent(EVENT_ERROR, reinterpret_cast(res)); } goto Lerror;