Copilot commented on code in PR #13804:
URL: https://github.com/apache/trafficserver/pull/13804#discussion_r4201574285
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -108,83 +142,172 @@ class force_VFPT_to_top
};
/**
- Base class for all state machines to receive notification of
- events.
-
- The Continuation class represents the main abstraction mechanism
- used throughout the IO Core Event System to communicate its users
- the occurrence of an event. A Continuation is a lightweight data
- structure that implements a single method with which the user is
- called back.
-
- Continuations are typically subclassed in order to implement
- event-driven state machines. By including additional state and
- methods, continuations can combine state with control flow, and
- they are generally used to support split-phase, event-driven
- control flow.
-
- Given the multithreaded nature of the Event System, every
- continuation carries a reference to a ProxyMutex object to protect
- its state and ensure atomic operations. This ProxyMutex object
- must be allocated by continuation-derived classes or by clients
- of the IO Core Event System and it is required as a parameter to
- the Continuation's class constructor.
-
+ Base class for event-driven state machines dispatched by the Event
+ System.
+
+ A Continuation pairs the @c handler that @c handleEvent invokes with
+ the @c mutex that serializes those invocations. Derive from it to add
+ state and handler methods; a derived class typically changes
+ @c handler as it moves between states.
+
+ @invariant @c handler is non-null whenever an event can be dispatched
+ to the Continuation.
+
+ @par Lifetime
+ Constructors are protected, so only derived classes are instantiated.
+ Once scheduled or passed to an asynchronous operation, a Continuation
+ must stay alive until no further event can be dispatched to it, i.e.,
+ until each such operation has completed or been cancelled. It may then be
+ destroyed through a @c Continuation pointer, including by
+ @c delete @c this from its own handler, provided nothing, including
+ callers further up the stack, accesses it afterward.
+
+ @par Thread Safety
+ Not thread-safe. While @c mutex is non-null, every access to the
+ Continuation, including calls to @c handleEvent, must be made while
+ holding it, except where a member documents otherwise; the Event
+ System holds it for the duration of each dispatch. While @c mutex is
+ null, callers must serialize access themselves.
*/
-
class Continuation : private force_VFPT_to_top
{
public:
/**
- The current continuation handler function.
-
- The current handler should not be set directly. In order to
- change it, first acquire the Continuation's lock and then use
- the SET_HANDLER macro which takes care of the type casting
- issues.
-
+ The current handler invoked by @c handleEvent.
+
+ Initial value is null; dispatching an event before a handler is
+ installed is undefined behavior. Install a handler with
+ @c SET_HANDLER (on @c this) or @c SET_CONTINUATION_HANDLER (on
+ another Continuation) rather than assigning directly; the macros
+ perform a type-checked conversion that a bare assignment skips,
+ catching offset bugs that would otherwise arise under multiple
+ inheritance.
+
+ @par Thread Safety
+ Unsynchronized pointer-to-member. Once the Continuation has been
+ published to any other thread, readers and writers MUST hold
+ @c this->mutex.
*/
ContinuationHandler handler = nullptr;
#ifdef DEBUG
+ /**
+ Name of the most recently installed handler, captured by
+ @c SET_HANDLER / @c SET_CONTINUATION_HANDLER for diagnostic use.
+ Present only in DEBUG builds. Initial value is null. Same
+ synchronization rules as @c handler.
+ */
const char *handler_name = nullptr;
#endif
/**
- The Continuation's lock.
-
- A reference counted pointer to the Continuation's lock. This
- lock is initialized in the constructor and should not be set
- directly.
-
- TODO: make this private.
-
+ Reference-counted pointer to the @c ProxyMutex protecting this
+ Continuation's state.
+
+ Initialized by the Continuation's constructor. The field MAY be
+ reassigned after construction, but only while no other thread is
+ dispatching this Continuation. Some scheduling paths assign the
+ dispatching thread's mutex to this field when it is null; others
+ leave a null mutex unchanged, in which case the Continuation
+ dispatches without serialization. A null value is permitted only
+ when dispatching through a Processor that documents the no-mutex
+ case.
+
+ @par Thread Safety
+ The reference itself is not synchronized. Reads and writes of the
+ field MUST be ordered by an external happens-before edge (typically
+ the publication of the Continuation to a Processor); concurrent
+ unsynchronized access is a data race. The lock macros
+ (@c MUTEX_TRY_LOCK, @c SCOPED_MUTEX_LOCK, etc.) accept the
+ @c Ptr<ProxyMutex> directly.
*/
Ptr<ProxyMutex> mutex;
+ /**
+ Returns a raw pointer to the @c ProxyMutex protecting this
+ Continuation, without changing the reference count.
+
+ @return The @c ProxyMutex currently held in @c this->mutex, or
+ nullptr if the field is null. The pointer is valid only
+ while @c this->mutex retains a reference to it; to keep
+ the mutex alive past the Continuation's destruction or a
+ reassignment of @c this->mutex, the caller MUST take its
+ own @c Ptr<ProxyMutex> rather than store the raw pointer.
+
+ @par Thread Safety
+ Caller-synchronized. Callers must order this read against any
+ concurrent writers via an external happens-before edge.
+ */
ProxyMutex *
getMutex() const
{
return mutex.get();
}
/**
- Link to other continuations.
-
- A doubly-linked element to allow Lists of Continuations to be
- assembled.
-
+ Doubly-linked list hook used to enqueue this Continuation in
+ intrusive lists whose list-traits class is the nested type
+ @c Continuation::Link_link (declared by the @c LINK macro). Both
+ @c next and @c prev are null-initialized by @c Link<Continuation>'s
+ own default constructor, leaving the hook in the unlinked state.
+
+ @par Thread Safety
+ Plain links. The owner of the list (the Processor or subsystem
+ that holds the queue) is responsible for synchronizing
+ insertion, removal, and traversal. Because there is a single
+ @c next / @c prev pair, the Continuation MUST belong to at most
+ one such list at a time.
*/
LINK(Continuation, link);
/**
- Contains values for debug_override and future flags that
- needs to be thread local while this continuation is running
+ Flags that follow this Continuation to whichever thread dispatches it.
+
+ Initialized from the constructing thread's current @c ContFlags.
+ Before each dispatch, the Event System makes these flags the
+ dispatching thread's current flags. Scheduling an event for this
+ Continuation MAY overwrite this field with the scheduling thread's
+ current flags, discarding earlier changes.
Review Comment:
“Before each dispatch” overstates the behavior. `EThread::process_event`
restores `control_flags` for regular queued events, but dedicated-thread
`start_event` dispatch and `EventProcessor::initThreadState` callbacks call
`handleEvent` without restoring them. Scope this guarantee to the regular
queued-event path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
