Copilot commented on code in PR #13804:
URL: https://github.com/apache/trafficserver/pull/13804#discussion_r4199390545
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -108,83 +142,181 @@ 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 IO Core
+ Event System.
+
+ A Continuation pairs a member-function handler with a @c ProxyMutex.
+ When a Processor delivers an event to the Continuation (via
+ @c handleEvent), the dispatching thread first acquires
+ @c this->mutex; the handler then runs while that lock is held and can
+ manipulate the Continuation's state safely. Subclasses add state and
+ additional handler methods, switching between them with
+ @c SET_HANDLER.
Review Comment:
The dispatcher does not always acquire a mutex: null-mutex Continuations are
supported, and `WeakMutexTryLock` treats null as successfully acquired without
locking (`Lock.h:505-520`). Qualify this statement so users do not assume
handlers for null-mutex Continuations are serialized.
This issue also appears in the following locations of the same file:
- line 166
- line 373
- line 392
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -195,30 +327,53 @@ 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, by convention
+ @c CONTINUATION_DONE or @c CONTINUATION_CONT. The Event
+ System dispatcher discards it; only direct callers of
+ @c handleEvent can give it meaning.
Review Comment:
The return value is not conventionally limited to DONE/CONT. Core handlers
also return values such as `EVENT_RETURN`, `EVENT_RESTART`, and `EVENT_ERROR`
(defined alongside the aliases in `Event.h` and used by cache/network
handlers). Restore the Processor-specific contract rather than implying only
two valid results.
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -108,83 +142,181 @@ 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 IO Core
+ Event System.
+
+ A Continuation pairs a member-function handler with a @c ProxyMutex.
+ When a Processor delivers an event to the Continuation (via
+ @c handleEvent), the dispatching thread first acquires
+ @c this->mutex; the handler then runs while that lock is held and can
+ manipulate the Continuation's state safely. Subclasses add state and
+ additional handler methods, switching between them with
+ @c SET_HANDLER.
+
+ @par Ownership
+ Caller-owned. Continuation does not allocate or free itself; the
+ derived state machine controls its own lifetime. Once a Continuation
+ has been registered with a Processor (e.g., via a @c schedule_*
+ call), it MUST remain alive until either (a) the Processor returns
+ @c ACTION_RESULT_DONE / @c ACTION_IO_ERROR for a synchronous
+ completion, or (b) every outstanding @c Action returned by that
+ Processor for this Continuation has been cancelled and any in-flight
+ callback has unwound.
+
+ @par Thread Safety
+ Not instance-thread-safe. All reads and writes of a Continuation's
+ fields MUST be performed by a thread that holds @c this->mutex, except
+ where an individual field documents otherwise. When the Event System
+ dispatches a handler it holds @c this->mutex for the duration of the
+ call; the dispatcher acquires that lock with a try-lock and reschedules
+ the Event rather than blocking, so a handler runs only once the lock is
+ available. Code reached from outside a handler MUST acquire the mutex
+ explicitly before touching the Continuation.
*/
-
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; scheduling a Continuation that holds a
+ null mutex causes the Event System to adopt the dispatching thread's
+ mutex. A null value is otherwise permitted only when dispatching
+ through a Processor that documents the no-mutex case.
Review Comment:
Scheduling does not generally adopt a mutex. Only `EThread::schedule`
performs that assignment (`UnixEThread.cc:520-524`); `EventProcessor::schedule`
and `EThread::schedule_local` leave a null Continuation mutex unchanged
(`UnixEventProcessor.cc:704-706`, `UnixEThread.cc:608-610`), after which
dispatch runs unlocked. Document the actual path-dependent behavior.
##########
include/iocore/eventsystem/Continuation.h:
##########
@@ -108,83 +142,181 @@ 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 IO Core
+ Event System.
+
+ A Continuation pairs a member-function handler with a @c ProxyMutex.
+ When a Processor delivers an event to the Continuation (via
+ @c handleEvent), the dispatching thread first acquires
+ @c this->mutex; the handler then runs while that lock is held and can
+ manipulate the Continuation's state safely. Subclasses add state and
+ additional handler methods, switching between them with
+ @c SET_HANDLER.
+
+ @par Ownership
+ Caller-owned. Continuation does not allocate or free itself; the
+ derived state machine controls its own lifetime. Once a Continuation
+ has been registered with a Processor (e.g., via a @c schedule_*
+ call), it MUST remain alive until either (a) the Processor returns
+ @c ACTION_RESULT_DONE / @c ACTION_IO_ERROR for a synchronous
+ completion, or (b) every outstanding @c Action returned by that
+ Processor for this Continuation has been cancelled and any in-flight
+ callback has unwound.
Review Comment:
This omits normal asynchronous completion as a valid end of the lifetime
requirement. After a one-shot operation has completed, `Action.h:77-83` says
its Action must not be accessed or cancelled, but this wording makes
cancellation the only non-inline endpoint. Include ordinary completion so
callers are not instructed to retain the Continuation indefinitely or cancel an
already-completed Action.
--
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]
