bneradt commented on code in PR #13804:
URL: https://github.com/apache/trafficserver/pull/13804#discussion_r4208712911
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -108,83 +146,157 @@ 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. Derived classes add
+ state and handler methods, and typically replace @c handler as they
+ move between states.
+
+ @par Lifetime
+ A handler must be installed before the Continuation is scheduled or
+ passed to an asynchronous operation. From then on, the 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, concurrent accesses must
+ hold it, except where a member documents otherwise; the Event System
+ holds it for the duration of each dispatch.
*/
-
class Continuation : private force_VFPT_to_top
{
public:
/**
- The current continuation handler function.
+ The member function that @c handleEvent invokes.
- 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.
+ Install a handler with @c SET_HANDLER or @c SET_CONTINUATION_HANDLER
+ rather than by direct assignment; see @c ContinuationHandler for why
+ a @c reinterpret_cast is unsafe. A handler may replace this field
+ while it runs; the new handler receives the next dispatch.
+ @par Thread Safety
+ Not thread-safe.
*/
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.
-
+ The mutex held while events are dispatched to this Continuation, or
+ null.
+
+ The Continuation shares ownership of the referenced @c ProxyMutex.
+ If the field is null when an event is scheduled, the scheduler may
+ choose a mutex to hold for that dispatch and may store it here, or
+ may dispatch without holding any mutex. Leave the field null only if
+ the Continuation's state needs no serialization and every
+ asynchronous operation it is passed to accepts a null mutex.
+
+ Reassigning the field while an event or asynchronous operation
+ targeting this Continuation is pending is undefined behavior.
+
+ @par Thread Safety
+ Not thread-safe. Holding the referenced mutex does not protect the
+ field itself, and scheduling this Continuation may write it.
*/
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
+ Control flags that may be installed as the current thread's control
+ flags before an event is dispatched to this Continuation.
+
+ Initialized from the constructing thread's current control flags.
+ Scheduling an event for this Continuation may overwrite this field
+ with the scheduling thread's current control flags.
+
+ @par Thread Safety
+ Not thread-safe. Scheduling and some dispatch paths access this
+ field without holding @c mutex, so holding @c mutex is not enough
+ to avoid a data race.
*/
ContFlags control_flags;
+ /**
+ The EThread on which this Continuation prefers to run, or nullptr
+ if no preference has been set.
+
+ Read by @c EventProcessor::schedule when choosing the thread to
+ service a Continuation, and by subsystems that pin work to a thread
+ (e.g., UDP, HostDB, and the plugin API). The field is advisory —
+ Processors are not required to honor it.
Review Comment:
This says `EventProcessor::schedule` *reads* the field, but it also writes
it. When `thread_affinity` is null, `schedule` calls
`e->continuation->setThreadAffinity(e->ethread)` (`UnixEventProcessor.cc`,
around line 700). So the first schedule pins the continuation to whichever
thread was picked, which matters to anyone reasoning about where later events
will run. Could this say something like "Read by `EventProcessor::schedule`,
which also sets it to the chosen thread when it is null"? The Thread Safety
paragraph below should probably mention that scheduling may write it too, the
same way the `mutex` docs do.
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -195,30 +307,56 @@ class Continuation : private force_VFPT_to_top
return false;
}
+ /**
+ Returns the EThread previously installed as this Continuation's
+ affinity, or nullptr if none has been set.
+
+ @par Thread Safety
+ Caller-synchronized read; see @c thread_affinity.
+ */
EThread *
getThreadAffinity()
{
return thread_affinity;
}
+ /**
+ Clears the dispatch-thread affinity, restoring the "no preference"
+ state.
+
+ @par Thread Safety
+ Caller-synchronized; see @c thread_affinity.
+ */
void
clearThreadAffinity()
{
thread_affinity = nullptr;
}
/**
- Receives the event code and data for an Event.
-
- This function receives the event code and data for an event and
- forwards them to the current continuation handler. The processor
- calling back the continuation is responsible for acquiring its
- lock. If the lock is present and not held, this method will assert.
-
- @param event Event code to be passed at callback (Processor specific).
- @param data General purpose data related to the event code (Processor
specific).
- @return State machine and processor specific return code.
-
+ Dispatches an event to this Continuation's currently installed
+ handler.
+
+ @param[in] event Event code to forward. Meaning is Processor-specific
+ (e.g., @c VC_EVENT_READ_READY, @c EVENT_IMMEDIATE).
+ Defaults to @c CONTINUATION_EVENT_NONE.
+ @param[in,out] data Auxiliary payload to forward. Lifetime, ownership, and
+ type are Processor-specific. Defaults to nullptr.
+ @return The handler's return value. The Event System dispatcher
+ discards it; only direct callers of @c handleEvent define
+ its meaning. @c CONTINUATION_DONE and @c CONTINUATION_CONT
+ are the base-level conventions; Processor-specific protocols
+ may define additional return values.
+
+ @pre @c this->handler is non-null. Calling with a null handler is
+ undefined behavior (invokes a null pointer-to-member).
+
+ @par Thread Safety
+ Caller-synchronized via @c this->mutex when non-null. The Event
+ System holds the mutex around its calls when @c this->mutex is
+ non-null; Continuations with a null mutex run without
+ serialization. Ad-hoc callers MUST hold @c this->mutex before
+ calling when it is non-null.
Review Comment:
It's worth stating that this is enforced: `handleEvent` does
`ink_release_assert(!mutex || mutex->thread_holding == this_ethread())`, so
calling without the lock aborts the process even in release builds. It isn't
just a contract violation.
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -230,22 +368,85 @@ class Continuation : private force_VFPT_to_top
protected:
/**
- Constructor of the Continuation object. It should not be used
- directly. Instead create an object of a derived type.
+ Constructs a Continuation protected by @p amutex.
- @param amutex Lock to be set for this Continuation.
+ The Continuation takes a shared reference to @p amutex. If no other
+ reference exists, the mutex is destroyed when the Continuation
+ releases its reference, so a raw pointer kept by the caller dangles
+ after that.
+ @param[in] amutex The mutex to protect this Continuation, or nullptr.
+ See @c mutex for the restrictions on dispatching a
+ Continuation whose mutex is null.
+
+ @pre @p amutex is null, or points to a live @c ProxyMutex created
+ as its documentation requires.
Review Comment:
"created as its documentation requires" makes the reader go find that
documentation. It would be clearer to name it directly, e.g. "allocated with
`new_ProxyMutex()`".
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -108,83 +146,157 @@ 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. Derived classes add
+ state and handler methods, and typically replace @c handler as they
+ move between states.
+
+ @par Lifetime
+ A handler must be installed before the Continuation is scheduled or
+ passed to an asynchronous operation. From then on, the 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, concurrent accesses must
+ hold it, except where a member documents otherwise; the Event System
+ holds it for the duration of each dispatch.
*/
-
class Continuation : private force_VFPT_to_top
{
public:
/**
- The current continuation handler function.
+ The member function that @c handleEvent invokes.
- 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.
+ Install a handler with @c SET_HANDLER or @c SET_CONTINUATION_HANDLER
+ rather than by direct assignment; see @c ContinuationHandler for why
+ a @c reinterpret_cast is unsafe. A handler may replace this field
+ while it runs; the new handler receives the next dispatch.
+ @par Thread Safety
+ Not thread-safe.
*/
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;
Review Comment:
FYI: `doc/Doxyfile` has `ENABLE_PREPROCESSING = YES` but `PREDEFINED` is
empty, so this `#ifdef DEBUG` member won't appear in the generated docs. If we
want it rendered, `PREDEFINED = DEBUG` would do it. That would also make
Doxygen use the DEBUG versions of `SET_HANDLER` and `SET_CONTINUATION_HANDLER`.
Probably fine to leave as is, but I wanted to flag it.
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -56,11 +52,53 @@ extern EThread *this_event_thread();
//
//////////////////////////////////////////////////////////////////////////////
+/**
+ The default event code passed to @c Continuation::handleEvent when no
+ Processor-specific code applies. Processors define their own non-zero
+ event codes (e.g., @c EVENT_IMMEDIATE, @c VC_EVENT_READ_READY) that
+ handlers dispatch on.
+*/
#define CONTINUATION_EVENT_NONE 0
+/**
+ Handler return code signaling, by convention, that the state machine has
+ finished processing this event. @c EVENT_DONE and @c VC_EVENT_DONE alias
+ this value.
+
+ The Event System dispatcher discards the handler's return value; it is
+ meaningful only to callers that invoke @c handleEvent directly and define
+ a convention for it.
+*/
#define CONTINUATION_DONE 0
+
+/**
+ Handler return code signaling, by convention, that the state machine has
+ not finished processing and expects further dispatches. @c EVENT_CONT and
+ @c VC_EVENT_CONT alias this value.
+
+ The Event System dispatcher discards the handler's return value; it is
+ meaningful only to callers that invoke @c handleEvent directly and define
+ a convention for it.
+*/
#define CONTINUATION_CONT 1
+/**
+ Pointer-to-member type of an event handler, as stored in
+ @c Continuation::handler.
+
+ The handler receives the event code and payload of each event
+ dispatched to the Continuation, and returns a Processor-specific
+ status.
+
+ @note A handler declared in a class derived from @c Continuation must be
+ converted to this type with @c static_cast, which applies the
+ base-class offset. Invoking the result of a @c reinterpret_cast
+ to this type is undefined behavior.
+
+ @note Invoking a converted handler is undefined behavior unless the
+ Continuation's dynamic type is, or derives from, the class that
+ declared the handler.
+*/
Review Comment:
These notes call invoking the result of a `reinterpret_cast` undefined
behavior, but `continuation_handler_void_ptr` (which `SET_HANDLER` uses) does
exactly that: it `reinterpret_cast`s `int (C::*)(int, T *)` to `int (C::*)(int,
void *)` before the `static_cast`. The `SET_HANDLER` docs further down admit
that the `T*` parameter is reinterpret-cast. I'd reword this to make the
distinction explicit:
1. Casting straight to `ContinuationHandler` with `reinterpret_cast` skips
the derived-to-base adjustment, so it's actually broken.
2. Changing `T*` to `void*` is formally UB too, but the codebase depends on
it as a convention.
Also, the body of `continuation_handler_void_ptr` has a comment saying
`Continuation` must be the *first* base under multiple inheritance, or the
compiler warns. That's a real constraint on derived classes, but it sits in a
body comment Doxygen won't render. Could you promote it into this note, or into
a `/** */` block on `continuation_handler_void_ptr`? "`static_cast` applies the
base-class offset" reads as if any base position is fine.
--
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]
