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/05/11 14:43:46 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7807: Address assert on captive_action
shinrich opened a new pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807
This addresses the ink_assert described in #7705 two ways. First, it always clears the cancelled bit in the captive_action before returning the captive action. Second, the HttpSM handlers that are storing references to the HttpCacheSM captive_action, use the new try_clear() action to clear the pending_action member without cancelling it if it matches the action passed into the handler. Based on the stack trace in the issue, the handler in frame #27 with this change will not mark the captive action cancelled when clearing it.
I'll try to spend some time to exercise this case, but I haven't run into this yet. So verification from those who can reproduce the case would be appreciated.
This closes #7705
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#issuecomment-840198528
Overall good, just a minor style bikeshed. This works in my testing.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r630602927
##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -283,6 +283,7 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key)
return ACTION_RESULT_DONE;
} else {
ink_assert(pending_action != nullptr || write_locked == true);
+ captive_action.cancelled = false; // Make sure not cancelled before we hand it out
return &captive_action;
Review comment:
Not about this change, but I'm not sure why this function returns `&captive_action` instead of `pending_action`. Above line 276, `pending_action` is assigned `Action *` from the `cacheProcessor.open_read`. Should this function return `pending_action` here?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich merged pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r630606260
##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -283,6 +283,7 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key)
return ACTION_RESULT_DONE;
} else {
ink_assert(pending_action != nullptr || write_locked == true);
+ captive_action.cancelled = false; // Make sure not cancelled before we hand it out
return &captive_action;
Review comment:
No, the captive action is the fixed storage for the action that the HttpCacheSM functions return.
If you look in HttpSM, you'll see
```
pending_action =
c_sm->open_write(&key, s_url, &t_state.hdr_info.client_request, object_read_info,
static_cast<time_t>((t_state.cache_control.pin_in_cache_for < 0) ? 0 : t_state.cache_control.pin_in_cache_for),
retry, allow_multiple);
```
So the HttpSM's pending_action is pointing at the HttpCacheSM captive_action. If for some reason, the HttpSM no longer wants to deal with the HttpCacheSM result, it can cancel the captive_action that it's pending action is pointing at.
The HttpCacheSM's pending action is returned from the scheduling subsystem and may even be null. So when the HttpCacheSM is cleaning up, it should be canceling the action it's pending_action is pointing at.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#issuecomment-839354387
This doesn't work for me. I still see the assertion failure if I run `h2spec -t -k -h 127.0.0.1 -p 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r631490803
##########
File path: proxy/http/HttpSM.h
##########
@@ -196,6 +196,13 @@ class PendingAction
{
return pending_action;
}
+ void
+ try_clear(Action *current_action)
Review comment:
`clear_if`? `clear_if_action_is`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#issuecomment-840602998
Pushed new commit with name change.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r630651719
##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -283,6 +283,7 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key)
return ACTION_RESULT_DONE;
} else {
ink_assert(pending_action != nullptr || write_locked == true);
+ captive_action.cancelled = false; // Make sure not cancelled before we hand it out
return &captive_action;
Review comment:
OK, HttpCacheSM's captive_action is an action for handling HttpCacheSM from HttpSM. Well, when HttpSM cancels HttpCacheSM's captive_action, what HttpCacheSM should do?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r630602927
##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -283,6 +283,7 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key)
return ACTION_RESULT_DONE;
} else {
ink_assert(pending_action != nullptr || write_locked == true);
+ captive_action.cancelled = false; // Make sure not cancelled before we hand it out
return &captive_action;
Review comment:
Not about this change, but I'm not sure why this function returns `&captive_action` instead of `pending_action`. Above line 276, `pending_action` is assigned `Action *` from the `cacheProcessor.open_read()`. Should this function return `pending_action` here?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r631853286
##########
File path: proxy/http/HttpSM.h
##########
@@ -196,6 +196,13 @@ class PendingAction
{
return pending_action;
}
+ void
+ try_clear(Action *current_action)
Review comment:
Ok will change to clear_if_action_is.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r631505298
##########
File path: proxy/http/HttpSM.h
##########
@@ -196,6 +196,13 @@ class PendingAction
{
return pending_action;
}
+ void
+ try_clear(Action *current_action)
Review comment:
+1 on `clear_if_action_is`. `try_clear` reminds me of `try_lock` and make me want to put it into if statement.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#issuecomment-842475921
Cherry-picked to v9.1.x branch.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#issuecomment-839861003
Sadly h2spec for me doesn't even trigger the logic that would return captive_action. I went down the path of eliminating the captive_action, see my now closed PR #7819.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#issuecomment-842475921
Cherry-picked to v9.1.x branch.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r630602927
##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -283,6 +283,7 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key)
return ACTION_RESULT_DONE;
} else {
ink_assert(pending_action != nullptr || write_locked == true);
+ captive_action.cancelled = false; // Make sure not cancelled before we hand it out
return &captive_action;
Review comment:
Not about this change, but I'm not sure why this function returns `&captive_action` instead of `pending_action`. Above line 273, `pending_action` is assigned `Action *` from the `cacheProcessor.open_read`. Should this function return `pending_action` here?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r631507651
##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -283,6 +283,7 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key)
return ACTION_RESULT_DONE;
} else {
ink_assert(pending_action != nullptr || write_locked == true);
+ captive_action.cancelled = false; // Make sure not cancelled before we hand it out
return &captive_action;
Review comment:
> The captive_action::cancel() passes through to cancel the pending_action member of the HttpCacheSM.
https://github.com/apache/trafficserver/pull/7819#issuecomment-839860155
I missed this part. I agree we can't get rid of `captive_action`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#discussion_r630602927
##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -283,6 +283,7 @@ HttpCacheSM::do_cache_open_read(const HttpCacheKey &key)
return ACTION_RESULT_DONE;
} else {
ink_assert(pending_action != nullptr || write_locked == true);
+ captive_action.cancelled = false; // Make sure not cancelled before we hand it out
return &captive_action;
Review comment:
Not about this change, but I'm not sure why this function return `&captive_action` instead of `pending_action`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7807: Address assert on captive_action
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7807:
URL: https://github.com/apache/trafficserver/pull/7807#issuecomment-840183014
Fortunately @SolidWallOfCode was able to make it fail, so tracked down why the action was being cancelled. Should be fixed now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org