Copilot commented on code in PR #13812:
URL: https://github.com/apache/trafficserver/pull/13812#discussion_r4207101797
##########
include/iocore/eventsystem/Action.h:
##########
@@ -28,113 +28,109 @@
#include "iocore/eventsystem/Continuation.h"
/**
- Represents an operation initiated on a Processor.
-
- The Action class is an abstract representation of an operation
- being executed by some Processor. A reference to an Action object
- allows you to cancel an ongoing asynchronous operation before it
- completes. This means that the Continuation specified for the
- operation will not be called back.
-
- Actions or classes derived from Action are the typical return
- type of methods exposed by Processors in the Event System and
- throughout the IO Core libraries.
-
- The canceller of an action must be the state machine that will
- be called back by the task and that state machine's lock must be
- held while calling cancel.
-
- Processor implementers:
-
- You must ensure that no events are sent to the state machine after
- the operation has been cancelled appropriately.
-
- Returning an Action:
-
- Processor functions that are asynchronous must return actions to
- allow the calling state machine to cancel the task before completion.
- Because some processor functions are reentrant, they can call
- back the state machine before the returning from the call that
- creates the actions. To handle this case, special values are
- returned in place of an action to indicate to the state machine
- that the action is already completed.
-
- - @b ACTION_RESULT_DONE The processor has completed the task
- and called the state machine back inline.
- - @b ACTION_RESULT_INLINE Not currently used.
- - @b ACTION_RESULT_IO_ERROR Not currently used.
-
- To make matters more complicated, it's possible if the result is
- ACTION_RESULT_DONE that state machine deallocated itself on the
- reentrant callback. Thus, state machine implementers MUST either
- use a scheme to never deallocate their machines on reentrant
- callbacks OR immediately check the returned action when creating
- an asynchronous task and if it is ACTION_RESULT_DONE neither read
- nor write any state variables. With either method, it's imperative
- that the returned action always be checked for special values and
- the value handled accordingly.
-
- Allocation policy:
-
- Actions are allocated by the Processor performing the actions.
- It is the processor's responsibility to handle deallocation once
- the action is complete or cancelled. A state machine MUST NOT
- access an action once the operation that returned the Action has
- completed or it has cancelled the Action.
-
- Action pointer sanity checks must also check whether the lowest
- bit of the pointer is 1. If it is 1, then the value must not be
- treated as a pointer, and should be used as one of the values
- defined below (e.g. ACTION_RESULT_DONE).
-
+ Handle to an in-flight asynchronous operation.
+
+ An Action is returned by a Processor when it accepts an asynchronous
+ request from a Continuation. Holding the Action lets the Continuation
+ cancel the operation before it completes; once cancelled, the Continuation
+ will not be called back for that operation.
+
+ Processors that derive from Action attach additional state to the handle.
+ Processors that complete a request synchronously (re-entrantly) MAY return
+ a sentinel @c Action* (see @c MAKE_ACTION_RESULT) instead of a real
+ pointer; callers MUST check the low bit of the returned pointer to
+ distinguish a real Action from a sentinel before dereferencing.
+
+ @par Ownership
+ Allocated by the Processor that returned the Action; deallocated by that
+ same Processor when the operation completes or is cancelled. Callers
+ MUST NOT delete an Action* and MUST NOT access an Action after the
+ operation it represents has completed or after they have called cancel().
+
+ @par Thread Safety
+ Not instance-thread-safe. The Continuation that initiated the Action is
+ the only legitimate canceller, and it MUST hold its own @c ProxyMutex
+ (the same mutex stored in @c Action::mutex) while calling cancel(). The
+ Processor MUST guarantee that no callbacks are delivered to a cancelled
+ Action.
*/
class Action
{
public:
/**
- Continuation that initiated this action.
-
- The reference to the initiating continuation is only used to
- verify that the action is being cancelled by the correct
- continuation. This field should not be accessed or modified
- directly by the state machine.
-
+ The Continuation that initiated this Action.
+
+ @par Thread Safety
+ The owning Processor binds this field (via @c operator= or by
+ direct assignment) before the Action is made observable to a
+ canceller, and the value is stable from that point until the
+ Processor releases the Action. Rebinding (including to nullptr)
+ is the Processor's responsibility and MUST be serialized against
+ any concurrent cancel().
*/
Continuation *continuation = nullptr;
/**
- Reference to the Continuation's lock.
-
- Keeps a reference to the Continuation's lock to preserve the
- access to the cancelled field valid even when the state machine
- has been deallocated. This field should not be accessed or
- modified directly by the state machine.
-
+ A retained reference to the initiating Continuation's @c ProxyMutex.
+
+ Held independently of @c continuation so that @c cancelled remains
+ accessible under a valid lock even after the initiating Continuation
+ has been deallocated.
+
+ @par Thread Safety
+ The owning Processor binds this field (via @c operator= or by
+ direct assignment) before the Action is made observable to a
+ canceller, and the value is stable from that point until the
+ Processor releases the Action. Rebinding (including to nullptr)
+ is the Processor's responsibility and MUST be serialized against
+ any concurrent cancel(). The retained reference keeps the
+ @c ProxyMutex alive for as long as it is held.
*/
Ptr<ProxyMutex> mutex;
/**
- Internal flag used to indicate whether the action has been
- cancelled.
-
- This flag is set after a call to cancel or cancel_action and
- it should not be accessed or modified directly by the state
- machine.
-
+ Set to true after cancel() or cancel_action() is invoked. Initially
+ false.
+
+ The owning Processor MAY clear this flag back to false when recycling
+ the Action for a new operation, before the recycled Action is
+ published to a canceller.
+
+ @par Thread Safety
+ Plain @c bool. Readers and writers MUST hold @c this->mutex, except
+ that the owning Processor MAY clear it without the lock while the
+ Action is not yet published to any canceller. The Processor MUST
+ inspect this flag under @c this->mutex immediately before invoking
+ @c continuation, and MUST NOT invoke @c continuation if the flag is
+ set.
*/
bool cancelled = false;
/**
- Cancels the asynchronous operation represented by this action.
+ Cancels the asynchronous operation represented by this Action.
+
+ After a successful return, no callback for this operation will be
+ delivered to @c continuation. Derived Processors may override this
+ method to release additional resources before flagging the Action as
+ cancelled.
Review Comment:
This both names the wrong subtype and imposes an ordering that existing
overrides do not follow. `HttpCacheAction::cancel()` sets `cancelled` before
calling `cancel_pending_action()` (`src/proxy/http/HttpCacheSM.cc:52-59`), so
derived `Action` cleanup can occur after flagging.
##########
include/iocore/eventsystem/Action.h:
##########
@@ -28,113 +28,109 @@
#include "iocore/eventsystem/Continuation.h"
/**
- Represents an operation initiated on a Processor.
-
- The Action class is an abstract representation of an operation
- being executed by some Processor. A reference to an Action object
- allows you to cancel an ongoing asynchronous operation before it
- completes. This means that the Continuation specified for the
- operation will not be called back.
-
- Actions or classes derived from Action are the typical return
- type of methods exposed by Processors in the Event System and
- throughout the IO Core libraries.
-
- The canceller of an action must be the state machine that will
- be called back by the task and that state machine's lock must be
- held while calling cancel.
-
- Processor implementers:
-
- You must ensure that no events are sent to the state machine after
- the operation has been cancelled appropriately.
-
- Returning an Action:
-
- Processor functions that are asynchronous must return actions to
- allow the calling state machine to cancel the task before completion.
- Because some processor functions are reentrant, they can call
- back the state machine before the returning from the call that
- creates the actions. To handle this case, special values are
- returned in place of an action to indicate to the state machine
- that the action is already completed.
-
- - @b ACTION_RESULT_DONE The processor has completed the task
- and called the state machine back inline.
- - @b ACTION_RESULT_INLINE Not currently used.
- - @b ACTION_RESULT_IO_ERROR Not currently used.
-
- To make matters more complicated, it's possible if the result is
- ACTION_RESULT_DONE that state machine deallocated itself on the
- reentrant callback. Thus, state machine implementers MUST either
- use a scheme to never deallocate their machines on reentrant
- callbacks OR immediately check the returned action when creating
- an asynchronous task and if it is ACTION_RESULT_DONE neither read
- nor write any state variables. With either method, it's imperative
- that the returned action always be checked for special values and
- the value handled accordingly.
-
- Allocation policy:
-
- Actions are allocated by the Processor performing the actions.
- It is the processor's responsibility to handle deallocation once
- the action is complete or cancelled. A state machine MUST NOT
- access an action once the operation that returned the Action has
- completed or it has cancelled the Action.
-
- Action pointer sanity checks must also check whether the lowest
- bit of the pointer is 1. If it is 1, then the value must not be
- treated as a pointer, and should be used as one of the values
- defined below (e.g. ACTION_RESULT_DONE).
-
+ Handle to an in-flight asynchronous operation.
+
+ An Action is returned by a Processor when it accepts an asynchronous
+ request from a Continuation. Holding the Action lets the Continuation
+ cancel the operation before it completes; once cancelled, the Continuation
+ will not be called back for that operation.
+
+ Processors that derive from Action attach additional state to the handle.
+ Processors that complete a request synchronously (re-entrantly) MAY return
+ a sentinel @c Action* (see @c MAKE_ACTION_RESULT) instead of a real
+ pointer; callers MUST check the low bit of the returned pointer to
+ distinguish a real Action from a sentinel before dereferencing.
+
+ @par Ownership
+ Allocated by the Processor that returned the Action; deallocated by that
+ same Processor when the operation completes or is cancelled. Callers
+ MUST NOT delete an Action* and MUST NOT access an Action after the
+ operation it represents has completed or after they have called cancel().
+
+ @par Thread Safety
+ Not instance-thread-safe. The Continuation that initiated the Action is
+ the only legitimate canceller, and it MUST hold its own @c ProxyMutex
+ (the same mutex stored in @c Action::mutex) while calling cancel(). The
+ Processor MUST guarantee that no callbacks are delivered to a cancelled
+ Action.
*/
class Action
{
public:
/**
- Continuation that initiated this action.
-
- The reference to the initiating continuation is only used to
- verify that the action is being cancelled by the correct
- continuation. This field should not be accessed or modified
- directly by the state machine.
-
+ The Continuation that initiated this Action.
+
+ @par Thread Safety
+ The owning Processor binds this field (via @c operator= or by
+ direct assignment) before the Action is made observable to a
+ canceller, and the value is stable from that point until the
+ Processor releases the Action. Rebinding (including to nullptr)
+ is the Processor's responsibility and MUST be serialized against
+ any concurrent cancel().
Review Comment:
This is not true for every published `Action`. For example,
`HttpCacheAction::init()` only stores its `HttpCacheSM` and publishes
`captive_action` without assigning this base field
(`include/proxy/http/HttpCacheSM.h:44-62,82-87`). Document this as the contract
for users of the base cancellation mechanism rather than an invariant of every
derived action.
This issue also appears in the following locations of the same file:
- line 80
- line 99
- line 120
- line 155
##########
include/iocore/eventsystem/Action.h:
##########
@@ -28,113 +28,109 @@
#include "iocore/eventsystem/Continuation.h"
/**
- Represents an operation initiated on a Processor.
-
- The Action class is an abstract representation of an operation
- being executed by some Processor. A reference to an Action object
- allows you to cancel an ongoing asynchronous operation before it
- completes. This means that the Continuation specified for the
- operation will not be called back.
-
- Actions or classes derived from Action are the typical return
- type of methods exposed by Processors in the Event System and
- throughout the IO Core libraries.
-
- The canceller of an action must be the state machine that will
- be called back by the task and that state machine's lock must be
- held while calling cancel.
-
- Processor implementers:
-
- You must ensure that no events are sent to the state machine after
- the operation has been cancelled appropriately.
-
- Returning an Action:
-
- Processor functions that are asynchronous must return actions to
- allow the calling state machine to cancel the task before completion.
- Because some processor functions are reentrant, they can call
- back the state machine before the returning from the call that
- creates the actions. To handle this case, special values are
- returned in place of an action to indicate to the state machine
- that the action is already completed.
-
- - @b ACTION_RESULT_DONE The processor has completed the task
- and called the state machine back inline.
- - @b ACTION_RESULT_INLINE Not currently used.
- - @b ACTION_RESULT_IO_ERROR Not currently used.
-
- To make matters more complicated, it's possible if the result is
- ACTION_RESULT_DONE that state machine deallocated itself on the
- reentrant callback. Thus, state machine implementers MUST either
- use a scheme to never deallocate their machines on reentrant
- callbacks OR immediately check the returned action when creating
- an asynchronous task and if it is ACTION_RESULT_DONE neither read
- nor write any state variables. With either method, it's imperative
- that the returned action always be checked for special values and
- the value handled accordingly.
-
- Allocation policy:
-
- Actions are allocated by the Processor performing the actions.
- It is the processor's responsibility to handle deallocation once
- the action is complete or cancelled. A state machine MUST NOT
- access an action once the operation that returned the Action has
- completed or it has cancelled the Action.
-
- Action pointer sanity checks must also check whether the lowest
- bit of the pointer is 1. If it is 1, then the value must not be
- treated as a pointer, and should be used as one of the values
- defined below (e.g. ACTION_RESULT_DONE).
-
+ Handle to an in-flight asynchronous operation.
+
+ An Action is returned by a Processor when it accepts an asynchronous
+ request from a Continuation. Holding the Action lets the Continuation
+ cancel the operation before it completes; once cancelled, the Continuation
+ will not be called back for that operation.
+
+ Processors that derive from Action attach additional state to the handle.
Review Comment:
`Processor` is not the subtype here; `Event`, `HttpCacheAction`, and other
handle classes derive from `Action`. As written, this gives readers the wrong
inheritance relationship.
This issue also appears on line 149 of the same file.
##########
include/iocore/eventsystem/Action.h:
##########
@@ -176,20 +210,92 @@ class Action
}
/**
- Constructor of the Action object. Processor implementers are
- responsible for associating this action with the proper
- Continuation.
+ Constructs an Action with no associated Continuation and no retained
+ mutex.
+
+ @pre None.
+ @post @c continuation is nullptr; @c mutex is null;
+ @c cancelled is false.
+
+ @par Errors
+ Cannot fail.
+ @par Thread Safety
+ Safe to call from any thread.
*/
Action() {}
+
+ /**
+ Releases the retained reference to the Continuation's mutex.
+
+ @pre The Action's owning Processor has completed its lifecycle for
+ this Action (the operation has finished or has been cancelled).
+ @post The retained @c ProxyMutex reference is dropped; if this was
+ the last reference, the @c ProxyMutex is destroyed.
+
+ @par Errors
+ Cannot fail.
+
+ @par Thread Safety
+ The owning Processor invokes destruction; it MUST guarantee no other
+ thread accesses the Action concurrently.
+ */
virtual ~Action() {}
};
+/**
+ Sentinel return value: the Processor completed the request inline and
+ has already invoked the Continuation. The caller MUST treat the returned
+ pointer as a tag, not a real @c Action*. Equivalent to
+ @c MAKE_ACTION_RESULT(1).
+
+ When a caller observes this sentinel, the Continuation may have been
+ deallocated during the inline callback; the caller MUST NOT touch any
+ state that the Continuation owned after seeing this value.
Review Comment:
`ACTION_RESULT_DONE` does not always mean that the callback already ran
inline. `QUICNetProcessor::connect_re()` can schedule the connection startup
and return this sentinel immediately
(`src/iocore/net/QUICNetProcessor.cc:216-223`), while its `NET_EVENT_OPEN`
callback is delivered only when the handshake later starts the application
(`QUICNetVConnection.cc:248-282`). Define the sentinel as the absence of a
cancellable handle and leave callback timing to each processor API.
--
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]
