[Bug libstdc++/92895] New: [libstdc++] stop_token conformance issues

[lewissbaker.opensource at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92895

            Bug ID: 92895
           Summary: [libstdc++] stop_token conformance issues
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: lewissbaker.opensource at gmail dot com
  Target Milestone: ---

The current implementation of std::stop_token in trunk has a few issues that
make it non-conformant with regards to the latest draft specification N4842.

The version I reviewed was:
https://github.com/gcc-mirror/gcc/blob/87f8610d5d61ceda80ca91be963bdf3e989af4f6/libstdc%2B%2B-v3/include/std/stop_token

Note that there is a reference implementation available here:
https://github.com/josuttis/jthread/blob/master/source/stop_token.hpp


stop_token::stop_possible()

  This implementation is just checking whether the stop_token has
  ownership of a shared-state.
  This is necessary as per [stoptoken.mem]/2.1, but insufficient.

  It also needs to take into account [stoptoken.mem]/2.2 which requires that
this
  method also return false when it does have ownership of a shared state but
where
  there is no associated stop_source.
  This generally requires keeping a separate ref-count for the number of
stop_source
  instances.

stop_token::_Stop_cb::_M_callback

  The function pointer type could be declared noexcept here.
  This might avoid some extra exception-handling codegen being generated in
  _M_request_stop() when this function pointer is invoked.

stop_token::_Stop_cb::_S_execute()

  It is valid for the invocation of a callback registered with the
stop_callback to
  end up calling the destructor of the stop_callback object from within the
callback.

  See [stopcallback.cons]/6 which says:
    "... If callback is executing on the current thread, then the destructor
does not block (3.6)
    waiting for the return from the invocation of callback. ..."

  The intent of this wording was to indicate that it is potentially valid for
the destructor of
  a stop_callback object to execute on the thread that is currently executing
that instance's
  registered callback. i.e. that the destructor might run inside the
user-provided callback.

  Thus the current logic that tries to update '__cb->_M_prev = __cb->_M_next =
nullptr;' after
  invoking the registered callback is thus potentially writing to a dangling
pointer. You would
  either need to update these pointers before invoking the callback or
otherwise somehow detect
  whether the destructor has been run during the invocation of the callback.

stop_token::_Stop_state_t::_M_request_stop()

  This method holds the _Stop_state_t::_M_mtx lock while invoking user-provided
callbacks.
  This prevents stop_callbacks from being deregistered concurrently while the
user-provided
  callback is executing.

  See [stopcallback.cons]/6 which says:
    "The destructor does not block waiting for the execution of another
callback
    registered by an associated stop_callback."

  This is necessary to prevent certain deadlock situations that would otherwise
arise were
  a callback to try to acquire a lock on some mutex that might currently be
held by another thread
  that is concurrently trying to deregister a stop_callback.

stop_source::operator=(stop_source&& rhs)

  This implementation just does a swap() which leaves rhs with the previous
state
  of *this rather than leaving rhs as an empty state.

  See [stopcallback.cons]/10 which says that this should instead be equivalent
to:
    stop_source(std::move(rhs)).swap(*this).

  ie. you first need to construct a temporary stop_source and call swap() with
that.

stop_callback::stop_callback(const stop_token&, Callback&&)
stop_callback::stop_callback(stop_token&&, Callback&&)

  These constructors are only supposed to throw exceptions that are thrown by
  the constructor of the callback object.

  See [stopcallback.cons]/5 which states:
    "Throws: Any exception thrown by the initialization of callback."

  This implementation calls the _Stop_state_t::_M_register_callback()
  method which is not declared noexcept and which calls std::mutex::lock()
which might
  throw a std::system_error if it failed to acquire the mutex lock.

  It might be easier to provide a noexcept implementation here if the
implementation was built
  on top of std::atomic and used std::atomic::wait()-based spin-locks instead
of std::mutex.


  These constructors also have a race-condition that fails to call the callback
being registered
  inline if request_stop() is called concurrently by another thread in-between
the call to
    if (__state->_M_stop_requested())
  and
    else if (__state->_M_register_callback(this))


stop_callback::~stop_callback()

  This needs to ensure that it blocks until the callback finishes executing if
the
  the callback is executing concurrently on another thread and that it does not
block
  if the callback is currently executing on the current thread.

  It also needs to ensure that it does not block waiting for the execution of
some
  other associated callback to complete.

  See [stopcallback.cons]/6 for more info.