You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/10/20 16:28:47 UTC
[GitHub] [trafficserver] SolidWallOfCode opened a new pull request #8443: Add thread safety to PendingAction operations.
SolidWallOfCode opened a new pull request #8443:
URL: https://github.com/apache/trafficserver/pull/8443
We have seen crashes in `PluginVC` due to race conditions on the pending action. My proposed fix is
* Use `PendingAction` to hold the pending action, instead of raw pointers.
* Make `PendingAction` assignments thread safe.
* Enable scheduling from `PendingAction`.
For `PluginVC` the pending action is updated only when lock acquisition fails, which guarantees it is done near a concurrency race. Essentially if the instance can't be locked, an event is scheduled for future access and this needs to be stored in the instance. Beyond that, the wake up event is scheduled and then assigned to the member, which is yet another race condition. The observed crashes appear to be due to the scheduled event being handled on another thread before the assignment completes, leading to an assertion failure that the handled event is stored in the `PluginVC` instance. There are several approaches to this, but I think the best option is to using `PendingAction` to schedule the pending action in a way that updates the instance before the event is scheduled. This will require access to event internals because currently there does not appear to
be a way to generically create an `Event` without scheduling it.
--
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
[GitHub] [trafficserver] bneradt commented on a change in pull request #8443: Add thread safety to PendingAction operations.
Posted by 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
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8443: Add thread safety to PendingAction operations.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8443:
URL: https://github.com/apache/trafficserver/pull/8443#discussion_r733075764
##########
File path: include/tscore/PendingAction.h
##########
@@ -89,22 +91,30 @@ 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 HttpSMN to permanently stall.
Review comment:
GAH!
--
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
[GitHub] [trafficserver] SolidWallOfCode merged pull request #8443: Add thread safety to PendingAction operations.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode merged pull request #8443:
URL: https://github.com/apache/trafficserver/pull/8443
--
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
[GitHub] [trafficserver] randall commented on a change in pull request #8443: Add thread safety to PendingAction operations.
Posted by GitBox <gi...@apache.org>.
randall commented on a change in pull request #8443:
URL: https://github.com/apache/trafficserver/pull/8443#discussion_r732979416
##########
File path: include/tscore/PendingAction.h
##########
@@ -89,22 +91,30 @@ 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 HttpSMN to permanently stall.
Review comment:
HttpSMN ?
--
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
[GitHub] [trafficserver] bneradt commented on a change in pull request #8443: Add thread safety to PendingAction operations.
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8443:
URL: https://github.com/apache/trafficserver/pull/8443#discussion_r738533219
##########
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:
Ah, right.
--
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
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8443: Add thread safety to PendingAction operations.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8443:
URL: https://github.com/apache/trafficserver/pull/8443#discussion_r738530972
##########
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:
If the instance has `pending_action == alpha` and this is called with `action == alpha` then the `while` will not execute because the expression will be false. `expected` will be set to `alpha` from `pending_action` and then the condition is `while (alpha != alpha)` which will finish immediately.
--
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