Loading src/connection.h +9 −0 Original line number Diff line number Diff line Loading @@ -165,6 +165,15 @@ namespace netplus { std::atomic_bool ReadPending{ false }; std::atomic_bool PeerClosed{ false }; std::atomic_bool Closing{ false }; // Guards the ONE-TIME actual teardown (DisconnectEvent + remove-from-registry + // close()), separately from Closing above. Several independent code paths (a // failed read while a write is still in flight, that write's own later // completion, the idle reaper, ...) each call in thinking they might be the one // responsible for finishing this connection off; Closing alone can't gate that // safely because a path that finds Closing already true has no way to tell // whether the path that set it actually finished the teardown or deferred it // (e.g. a write was still pending) -- see event/iocp.cpp's finish_close(). std::atomic_bool Closed{ false }; std::atomic_bool ResponsePending{ false }; std::atomic_int PendingOps{ 0 }; }; Loading src/event/iocp.cpp +39 −26 Original line number Diff line number Diff line Loading @@ -117,20 +117,45 @@ namespace netplus { st->sockToCon.erase(s); } static bool mark_closing(con* c) { bool expected = false; return c->slots[0].Closing.compare_exchange_strong(expected, true); } static bool try_cleanup_con(event* ev, EventState* st, con* owner, SOCKET cs, int tid) { if (!mark_closing(owner)) return false; // Idempotent -- always safe to call regardless of whether some other path already set // this. Deliberately does NOT gate the actual teardown (see finish_close() below): a // caller that finds Closing already true still needs to know whether IT is the one that // should finish the job now (e.g. a write it was waiting on just completed). static void mark_closing(con* c) { c->slots[0].Closing.store(true); } // Performs the once-only teardown (DisconnectEvent + remove_con + close()) once nothing // is still in flight. Safe to call from multiple independent code paths that each think // they might be the one responsible for finishing this connection off -- e.g. a failed // read completion that finds a write still pending defers to that write's own eventual // completion to finish the job. Both call this; Closed's compare-exchange ensures only // one of them actually performs it, whichever happens to find WritePending already clear. // // Previously this was folded into try_cleanup_con() as "if mark_closing() fails (Closing // already true), give up" -- which silently dropped the connection forever whenever the // path that first set Closing had to defer (WritePending true at ITS time): the SECOND // path (e.g. that same write's completion) would find Closing already true, conclude // someone else already handled cleanup, and also do nothing. Neither ever called // DisconnectEvent/remove_con/close() -- a real connection + socket handle leak on every // read-failure-races-a-pending-write (or equivalent) sequence, which a busy server under // any real network hiccups hits constantly. reapIdleConnections()'s sweep couldn't // recover these either, since it went through the same broken check. static bool finish_close(event* ev, EventState* st, con* owner, SOCKET cs, int tid) { if (owner->slots[0].WritePending.load()) return false; bool expected = false; if (!owner->slots[0].Closed.compare_exchange_strong(expected, true)) return false; ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try { owner->slots[0].csock->close(); } catch (...) {} return true; } static bool try_cleanup_con(event* ev, EventState* st, con* owner, SOCKET cs, int tid) { mark_closing(owner); return finish_close(ev, st, owner, cs, tid); } // Test-only escape hatch, mirrors epoll.cpp/kqueue.cpp's identical hook (same env var) -- // a real deployment never sets this, so this returns kIdleSweepIntervalSeconds (30) // unconditionally. Exists purely so an idle-reaper regression test isn't forced to block Loading Loading @@ -499,28 +524,20 @@ namespace netplus { } if (!ok) { // Unreachable in practice (the top-level "Handle failed IOCP // completion" branch above already continues past this point for // any !ok completion) -- kept as a defensive fallback rather than // assuming that invariant always holds, using the same guarded // teardown as everywhere else. owner->slots[0].ReadPending.store(false); if (mark_closing(owner)) { if (!owner->slots[0].WritePending.load()) { ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try { owner->slots[0].csock->close(); } catch (...) {} } } try_cleanup_con(ev, st, owner, cs, tid); continue; } if (bytes == 0) { owner->slots[0].ReadPending.store(false); owner->slots[0].PeerClosed.store(true); if (!mark_closing(owner)) continue; if (owner->slots[0].WritePending.load()) continue; try { owner->slots[0].csock->close(); } catch (...) {} if (owner->slots[0].WritePending.load() || owner->slots[0].csock->hasPendingWrite()) continue; ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try_cleanup_con(ev, st, owner, cs, tid); continue; } Loading Loading @@ -637,9 +654,7 @@ namespace netplus { if (owner->slots[0].Closing.load()) { owner->slots[0].WritePending.store(false); ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try { owner->slots[0].csock->close(); } catch (...) {} finish_close(ev, st, owner, cs, tid); continue; } Loading Loading @@ -735,9 +750,7 @@ namespace netplus { } if (owner->slots[0].Closing.load()) { ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try { owner->slots[0].csock->close(); } catch (...) {} finish_close(ev, st, owner, cs, tid); continue; } Loading Loading
src/connection.h +9 −0 Original line number Diff line number Diff line Loading @@ -165,6 +165,15 @@ namespace netplus { std::atomic_bool ReadPending{ false }; std::atomic_bool PeerClosed{ false }; std::atomic_bool Closing{ false }; // Guards the ONE-TIME actual teardown (DisconnectEvent + remove-from-registry + // close()), separately from Closing above. Several independent code paths (a // failed read while a write is still in flight, that write's own later // completion, the idle reaper, ...) each call in thinking they might be the one // responsible for finishing this connection off; Closing alone can't gate that // safely because a path that finds Closing already true has no way to tell // whether the path that set it actually finished the teardown or deferred it // (e.g. a write was still pending) -- see event/iocp.cpp's finish_close(). std::atomic_bool Closed{ false }; std::atomic_bool ResponsePending{ false }; std::atomic_int PendingOps{ 0 }; }; Loading
src/event/iocp.cpp +39 −26 Original line number Diff line number Diff line Loading @@ -117,20 +117,45 @@ namespace netplus { st->sockToCon.erase(s); } static bool mark_closing(con* c) { bool expected = false; return c->slots[0].Closing.compare_exchange_strong(expected, true); } static bool try_cleanup_con(event* ev, EventState* st, con* owner, SOCKET cs, int tid) { if (!mark_closing(owner)) return false; // Idempotent -- always safe to call regardless of whether some other path already set // this. Deliberately does NOT gate the actual teardown (see finish_close() below): a // caller that finds Closing already true still needs to know whether IT is the one that // should finish the job now (e.g. a write it was waiting on just completed). static void mark_closing(con* c) { c->slots[0].Closing.store(true); } // Performs the once-only teardown (DisconnectEvent + remove_con + close()) once nothing // is still in flight. Safe to call from multiple independent code paths that each think // they might be the one responsible for finishing this connection off -- e.g. a failed // read completion that finds a write still pending defers to that write's own eventual // completion to finish the job. Both call this; Closed's compare-exchange ensures only // one of them actually performs it, whichever happens to find WritePending already clear. // // Previously this was folded into try_cleanup_con() as "if mark_closing() fails (Closing // already true), give up" -- which silently dropped the connection forever whenever the // path that first set Closing had to defer (WritePending true at ITS time): the SECOND // path (e.g. that same write's completion) would find Closing already true, conclude // someone else already handled cleanup, and also do nothing. Neither ever called // DisconnectEvent/remove_con/close() -- a real connection + socket handle leak on every // read-failure-races-a-pending-write (or equivalent) sequence, which a busy server under // any real network hiccups hits constantly. reapIdleConnections()'s sweep couldn't // recover these either, since it went through the same broken check. static bool finish_close(event* ev, EventState* st, con* owner, SOCKET cs, int tid) { if (owner->slots[0].WritePending.load()) return false; bool expected = false; if (!owner->slots[0].Closed.compare_exchange_strong(expected, true)) return false; ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try { owner->slots[0].csock->close(); } catch (...) {} return true; } static bool try_cleanup_con(event* ev, EventState* st, con* owner, SOCKET cs, int tid) { mark_closing(owner); return finish_close(ev, st, owner, cs, tid); } // Test-only escape hatch, mirrors epoll.cpp/kqueue.cpp's identical hook (same env var) -- // a real deployment never sets this, so this returns kIdleSweepIntervalSeconds (30) // unconditionally. Exists purely so an idle-reaper regression test isn't forced to block Loading Loading @@ -499,28 +524,20 @@ namespace netplus { } if (!ok) { // Unreachable in practice (the top-level "Handle failed IOCP // completion" branch above already continues past this point for // any !ok completion) -- kept as a defensive fallback rather than // assuming that invariant always holds, using the same guarded // teardown as everywhere else. owner->slots[0].ReadPending.store(false); if (mark_closing(owner)) { if (!owner->slots[0].WritePending.load()) { ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try { owner->slots[0].csock->close(); } catch (...) {} } } try_cleanup_con(ev, st, owner, cs, tid); continue; } if (bytes == 0) { owner->slots[0].ReadPending.store(false); owner->slots[0].PeerClosed.store(true); if (!mark_closing(owner)) continue; if (owner->slots[0].WritePending.load()) continue; try { owner->slots[0].csock->close(); } catch (...) {} if (owner->slots[0].WritePending.load() || owner->slots[0].csock->hasPendingWrite()) continue; ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try_cleanup_con(ev, st, owner, cs, tid); continue; } Loading Loading @@ -637,9 +654,7 @@ namespace netplus { if (owner->slots[0].Closing.load()) { owner->slots[0].WritePending.store(false); ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try { owner->slots[0].csock->close(); } catch (...) {} finish_close(ev, st, owner, cs, tid); continue; } Loading Loading @@ -735,9 +750,7 @@ namespace netplus { } if (owner->slots[0].Closing.load()) { ev->DisconnectEvent(*owner, tid, 0); remove_con(st, cs); try { owner->slots[0].csock->close(); } catch (...) {} finish_close(ev, st, owner, cs, tid); continue; } Loading