Copilot commented on code in PR #13804: URL: https://github.com/apache/trafficserver/pull/13804#discussion_r4199559272
########## include/iocore/eventsystem/Continuation.h: ########## @@ -108,83 +142,188 @@ 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 acquires @c this->mutex when + it is non-null; the handler then runs while that lock is held and can + manipulate the Continuation's state safely. Continuations with a null + @c mutex run without serialization. Subclasses add state and + additional handler methods, switching between them with + @c SET_HANDLER. Review Comment: This overstates the locking guarantee: synchronous processor paths such as `Cache::lookup` call `handleEvent` directly and rely on the calling thread to already hold the continuation mutex; the processor does not acquire it. Document the `handleEvent` precondition separately from the Event System dispatcher's behavior so callers do not assume arbitrary Processor callbacks add locking. This issue also appears in the following locations of the same file: - line 216 - line 402 ########## include/iocore/eventsystem/Continuation.h: ########## @@ -108,83 +142,188 @@ 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 acquires @c this->mutex when + it is non-null; the handler then runs while that lock is held and can + manipulate the Continuation's state safely. Continuations with a null + @c mutex run without serialization. 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 either completed (its callback + has returned) or been cancelled, and any in-flight callback has + unwound. Review Comment: Returning from a callback does not complete a recurring Event: it is re-enqueued after every callback and remains outstanding until cancellation. As written, this lifetime rule could lead a caller to destroy the Continuation after the first periodic callback and leave the recurring Event with a dangling pointer; distinguish final completion from an individual callback return. -- 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]
