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.
