Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: In Reactor Add A Mutex To Protect HandlerList Internal Array #1229

Merged
merged 9 commits into from
Sep 6, 2024

Commits on Aug 20, 2024

  1. Fix: In Reactor Add A Mutex To Protect HandlerList Internal Array

    Prompted by @tyler92's suggested PR#1228 "fix: data race on
    client/server shutdown":
    
    Looking at reactor.cc HandlerList, it does not attempt any multithread
    protection for its underlying std::array. In this experimental branch,
    we have added a mutex to HandlerList, and required that HandlerList
    members that access the array claim that mutex.
    
    Other related changes:
    
    Removed HandlerList default move constructors; mutex has a deleted
    move constructor.
    
    Removed HandlerList::clone(), an unused method of HandlerList that
    required a move constructor.
    
    Added a non-const form of "at" in HandlerList; further explanation in
    the comments.
    
    Guard the mutex for HandlerList::add, removeAll, at, and forEachHandler
    dgreatwood committed Aug 20, 2024
    Configuration menu
    Copy the full SHA
    ca26c0d View commit details
    Browse the repository at this point in the history

Commits on Aug 22, 2024

  1. Debug: Add Debug Logging for poller.reg_unreg_mutex_

    Added debug logging for poller.reg_unreg_mutex_ using the
    GUARD_AND_DBG_LOG macro.
    
    Made "std::mutex handlers_arr_mutex_" mutable for easier const
    function handling.
    dgreatwood committed Aug 22, 2024
    Configuration menu
    Copy the full SHA
    0058acd View commit details
    Browse the repository at this point in the history

Commits on Aug 30, 2024

  1. Fix: Revert Use of handlers_arr_mutex_ and Use poller.reg_unreg_mutex…

    …_ Instead
    
    Reason for Reversion
    ====================
    
    handlers_arr_mutex_ and poller.reg_unreg_mutex_ were getting into a
    deadlock with each other.
    - In Reactor::~Reactor(), HandlerList::forEachHandler (called from
      detachAndRemoveAllHandlers) would lock handlers_arr_mutex_, and then
      detachFromReactor would lock poller.reg_unreg_mutex_.
    - Meanwhile, SyncImpl::runOnce() locks poller.reg_unreg_mutex_ and
      then calls SyncImpl::handleFds which then calls HandlerList::at,
      locking handlers_arr_mutex_.
    Since the two locks are in opposite order, of course we have a
    deadlock if they happen simultaneously.
    
    Further review suggests that handlers_arr_mutex_ can be removed,
    provided we use poller.reg_unreg_mutex_ to guard the dangerous
    accesses to the handlers_ list.
    
    That's the strategy we've implemented on this commit.
    
    Specifics
    =========
    
    handlers_arr_mutex_ removed
    
    In os.h, reg_unreg_mutex_ made mutable so it can be locked in const
    functions.
    
    In reactor.cc:
    
    In SyncImpl::addHandler we lock poller.reg_unreg_mutex_ (addHandler
    calls handlers_.add, which of course modifies handlers_ and so
    requires protection).
    
    In SyncImpl::detachFromReactor, remove lock of
    poller.reg_unreg_mutex_. We are now locking poller.reg_unreg_mutex_ in
    detachAndRemoveAllHandlers instead, detachAndRemoveAllHandlers being
    the function that calls detachFromReactor.
    
    As above, in detachAndRemoveAllHandlers, lock
    poller.reg_unreg_mutex_. reg_unreg_mutex_ must be locked here since
    detachAndRemoveAllHandlers calls handlers_.forEachHandler, which
    requires handlers_ to be protected.
    
    Remove mutex locks from HandlerList functions "add", "removeAll",
    "at", and "forEachHandler". Also removed the HandlerList function
    "operator[]" (a synonym of "at") since it is not used and, I found,
    it made it harder to browse the code.
    dgreatwood committed Aug 30, 2024
    Configuration menu
    Copy the full SHA
    382de53 View commit details
    Browse the repository at this point in the history
  2. Update Version Date

    dgreatwood committed Aug 30, 2024
    Configuration menu
    Copy the full SHA
    575f9f0 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    28d2643 View commit details
    Browse the repository at this point in the history

Commits on Sep 5, 2024

  1. Fix: Lock interest_mutex_ When interest_ Modified in ctlEx

    EventMethEpollEquivImpl::ctlEx now locks interest_mutex_ when
    performing its add/mod/del actions.
    
    Previously, the interest_mutex_ lock was released before these
    add/mod/del actions were performed. This was confirmed to occasionally
    cause a "use after free" in findEmEventInAnInterestSet (called out of
    getReadyEmEvents) in another thread. It might also result in
    corruption of the interest_ std::set.
    dgreatwood committed Sep 5, 2024
    Configuration menu
    Copy the full SHA
    a255fac View commit details
    Browse the repository at this point in the history
  2. build: set soname to major.minor

    Until the 1.0 release, setting the soname to version_major.version_minor
    gives us more freedom to make changes without being too restricted by
    ABI compatibility.
    
    We never actually really commited to ABI stability before the 1.0
    relase - the README states that at the bottom - but I made this decision
    clearer by updating the "versioning" section.
    Tachi107 authored and dgreatwood committed Sep 5, 2024
    Configuration menu
    Copy the full SHA
    5e80557 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    b3e42cb View commit details
    Browse the repository at this point in the history

Commits on Sep 6, 2024

  1. Configuration menu
    Copy the full SHA
    963a6d3 View commit details
    Browse the repository at this point in the history