[GitHub] [trafficserver] bneradt commented on a change in pull request #8443: Add thread safety to PendingAction operations.

[GitBox <gi...@apache.org>]

bneradt commented on a change in pull request #8443:
URL: https://github.com/apache/trafficserver/pull/8443#discussion_r738503446



##########
File path: include/tscore/PendingAction.h
##########
@@ -89,22 +91,32 @@ PendingAction::empty() const
 inline PendingAction &
 PendingAction::operator=(Action *action)
 {
-  // Apparently HttpSM depends on not canceling the previous action if anew
+  // Apparently @c HttpSM depends on not canceling the previous action if a new
   // one completes immediately. Canceling the contained action in that case
-  // cause the HttpSm to permanently stall.
+  // cause the @c HttpSM to permanently stall.
   if (ACTION_RESULT_DONE != action) {
-    if (action != pending_action && pending_action != nullptr) {
-      pending_action->cancel();
+    Action *expected; // Need for exchange, and to load @a pending_action only once.
+    // Avoid race conditions - for each assigned action, ensure exactly one thread
+    // cancels it. Assigning @a expected in the @c while expression avoids potential
+    // races if two calls to this method have the same @a action.
+    while ((expected = pending_action) != action) {

Review comment:
       It looks like this will infinite loop if the user passes in the same `action` managed by `this` (i.e., the same as `pending_action`). The previous code checked and handled this situation.




-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org