Copilot commented on code in PR #13812:
URL: https://github.com/apache/trafficserver/pull/13812#discussion_r4207139828
##########
include/iocore/eventsystem/Action.h:
##########
@@ -27,114 +27,89 @@
#include "iocore/eventsystem/Thread.h"
#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 for cancelling a pending asynchronous operation.
+
+ A function that starts an asynchronous operation on behalf of a
+ Continuation returns an @c Action* for that operation. Calling cancel()
+ before the operation completes guarantees that @c continuation receives
+ no further callbacks for it.
+
+ A function returning @c Action* may instead return nullptr or a sentinel
+ value it documents; neither may be dereferenced.
+ @par Lifetime
+ The Action is owned by the object performing the operation, which
+ releases it after the operation completes or is cancelled. Callers must
+ not delete an Action. Except for the owning object, nothing may access
+ an Action after calling cancel() or after the operation completes.
+
+ @par Thread Safety
+ Not thread-safe. Callers of cancel() must hold @c mutex.
*/
class Action
{
public:
/**
- Continuation that initiated this action.
+ The Continuation called back for this operation, or nullptr if none.
- 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 Action does not keep this Continuation alive; it must remain valid
+ until the operation completes or is cancelled. Only the owning
+ Processor may modify this field.
+ @par Thread Safety
+ Not thread-safe.
*/
Continuation *continuation = nullptr;
/**
- Reference to the Continuation's lock.
+ The mutex that guards @c cancelled.
- 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.
+ Lock this mutex, not @c continuation->mutex, to cancel the Action.
+ The two usually refer to the same @c ProxyMutex, but a Processor may
+ bind a different one. Because the reference is retained, the mutex
+ stays valid after the initiating Continuation is deallocated.
Review Comment:
This suggests that using a mutex different from `continuation->mutex` is
supported, but callback delivery relies on those mutexes being identical.
HostDB explicitly treats a mismatch as memory corruption
(`src/iocore/hostdb/HostDB.cc:1014-1024`), and `operator=` establishes the
same-mutex invariant below. Document that invariant rather than permitting a
different binding; otherwise callers could believe cancellation is serialized
when the callback uses another lock.
--
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]
