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/03/31 21:19:12 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7667: Add class to normalize handling of pending action
shinrich opened a new pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667
We've been running with this code change for a couple months. @SolidWallOfCode suggested the PendingAction class to normalize how we store and clear the pending_action member of the HttpSM. For a long time we have had background level of crashes mostly from HostDB events that pop up long after the HttpSM has been deleted (and likely reallocated). The application of this class in our ATS9 has eliminated crashes like the following.
```
[ 00 ] libc-2.17.so raise ( raise.c:55 )
[ 01 ] libc-2.17.so abort ( abort.c:90 )
[ 02 ] libtscore.so.9.0.1 ink_abort ( ink_error.cc:99 )
[ 03 ] libtscore.so.9.0.1 _ink_assert ( ink_assert.cc:37 )
[ 04 ] traffic_server HostDBContinuation::probeEvent(int, Event*) ( HostDB.cc:1548 )
[ 05 ] traffic_server EThread::process_event(Event*, int) ( I_Continuation.h:167 )
[ 06 ] traffic_server EThread::execute_regular() ( UnixEThread.cc:241 )
[ 07 ] traffic_server execute ( UnixEThread.cc:332 )
[ 08 ] traffic_server EThread::execute() ( UnixEThread.cc:310 )
[ 09 ] traffic_server spawn_thread_internal ( Thread.cc:92 )
[ 10 ] libpthread-2.17.so start_thread ( pthread_create.c:307 )
```
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-814921664
I think inheriting from `ats_scoped_resource` is irrelevant, as that is simply an implementation convenience. The class behaves very much like the other scoped resources and that would also provide a natural place to put the implementation so it can be used in other classes.
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-813453797
Unfortunately we can't actually use `std::unique_ptr` because of the magic `ACTION_DONE` handling. Once the handling for that is implemented, there's no benefit to involving `std::unique_ptr`.
If you want a more consistent name, that would be `ats_scoped_action` in parallel to `ats_scoped_fd`, `ats_scoped_str`, `ats_scoped_mem`, `ats_scoped_obj`, etc., from "ink_memory.h".
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#discussion_r605294935
##########
File path: proxy/http/HttpSM.h
##########
@@ -166,6 +166,52 @@ enum HttpPluginTunnel_t {
class PluginVCCore;
+class PendingAction
+{
+public:
+ bool
+ operator==(Action *b)
+ {
+ return b == pending_action;
+ }
+ bool
+ operator!=(Action *b)
+ {
+ return b != pending_action;
+ }
+ PendingAction &
+ operator=(Action *b)
+ {
+ // Don't do anything if the new action is _DONE
+ if (b != ACTION_RESULT_DONE) {
+ if (b != pending_action && pending_action != nullptr) {
+ pending_action->cancel();
+ }
+ pending_action = b;
+ }
+ return *this;
+ }
+ Action *
+ operator->()
+ {
+ return pending_action;
+ }
+ Action *
+ get()
+ {
+ return pending_action;
+ }
+ ~PendingAction()
Review comment:
Is this destructor really called? It'd be called if HttpSM's destructor was called, but I don't think HttpSM's destructor is called because the allocator declaration don't have `true` to call the destructor automatically and I don't see any places that calls the destructor manually.
```
ClassAllocator<HttpSM> httpSMAllocator("httpSMAllocator");
```
--
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] bneradt removed a comment on pull request #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-814192613
[approve ci]
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#discussion_r605289503
##########
File path: proxy/http/HttpSM.h
##########
@@ -166,6 +166,52 @@ enum HttpPluginTunnel_t {
class PluginVCCore;
+class PendingAction
+{
+public:
+ bool
+ operator==(Action *b)
Review comment:
Nitpick: Why `b` but not `a`?
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-832815785
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] maskit commented on pull request #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-812283949
I wouldn't complain if the class name was PendingActionPtr. My point is that the current code *looks like* comparing something not a pointer with a pointer. It would be really nice if we could `std::unique_pointer` because anybody can easily see how it works.
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#discussion_r605765336
##########
File path: proxy/http/HttpSM.h
##########
@@ -166,6 +166,52 @@ enum HttpPluginTunnel_t {
class PluginVCCore;
+class PendingAction
+{
+public:
+ bool
+ operator==(Action *b)
+ {
+ return b == pending_action;
+ }
+ bool
+ operator!=(Action *b)
+ {
+ return b != pending_action;
+ }
+ PendingAction &
+ operator=(Action *b)
+ {
+ // Don't do anything if the new action is _DONE
+ if (b != ACTION_RESULT_DONE) {
+ if (b != pending_action && pending_action != nullptr) {
+ pending_action->cancel();
+ }
+ pending_action = b;
+ }
+ return *this;
+ }
+ Action *
+ operator->()
+ {
+ return pending_action;
+ }
+ Action *
+ get()
+ {
+ return pending_action;
+ }
+ ~PendingAction()
Review comment:
Good point. The pending_action is explicitly assigned to nullptr in the HttpSM::kill_this(). But you are correct. In the current code, the destructor is not called.
--
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 edited a comment on pull request #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
maskit edited a comment on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-813760982
`ats_scoped_action` sounds better to me in terms of naming, but it would cause another confusion. All `ats_scoped_*` are derived from `ats_scoped_resource`. Just adding `is_empty()` would be the simplest solution.
Now I have another concern.
```
PendingAction pa = some_function();
pa->cancel() // A
pa = nullptr; // B
```
The two lines, A and B, would basically the same thing, but line A does not clear `PendingAction::pending_action`. So `pa == nullptr` and `pa.is_empty()` would not return `false`. This is confusing, and can easily make bugs. I think we should not allow directly accessing the raw `Action *` in `PendingAction`.
--
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 edited a comment on pull request #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
maskit edited a comment on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-813760982
`ats_scoped_action` sounds better to me in terms of naming, but it would cause another confusion. All `ats_scoped_*` are derived from `ats_scoped_resource`. Just adding `is_empty()` would be the simplest solution.
Now I have another concern.
```
PendingAction pa = some_function();
pa->cancel() // A
pa = nullptr; // B
```
The two lines, A and B, would basically the same thing, but line A does not clear `PendingAction::pending_action`. So `pa == nullptr` and `pa.is_empty()` would not return `true`. This is confusing, and can easily make bugs. I think we should not allow directly accessing the raw `Action *` in `PendingAction`.
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-813760982
`ats_scoped_action` sounds better to me in terms of naming, but it would cause another confusion. All `ats_scoped_*` are derived from `ats_scoped_resource`. Just adding `is_empty()` would be the simplest solution.
Now I have another concern.
```
PendingAction pa = some_function();
pa->cancel() // A
pa = nullptr; // B
```
The two lines, A and B, would basically the same thing, but line A does not clear `PendingAction::pending_action`. `pa == nullptr` and `pa.is_empty()` would not return `false`. This is confusing, and can easily make bugs. I think we should not allow directly accessing the raw `Action *` in `PendingAction`.
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-813448483
Could we have a custom assigner for std::unique_ptr too?
--
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 edited a comment on pull request #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
maskit edited a comment on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-812283949
I wouldn't complain if the class name was PendingActionPtr. My point is that the current code *looks like* comparing something not a pointer with a pointer. It would be really nice if we could use `std::unique_pointer` because anybody can easily see how it works.
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-814180706
Pushed a commit to add is_empty, remove the equivalence operator, and remove the arrow operator. Also added a get_continuation() method to get around the absence of the arrow operator.
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-815005388
I assume there will be another PR in the future to move this out of HttpSM.h to a more general area. We can make another pass on naming discussions then.
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-832837601
@zwoop It might have been a bit too early to cherry-pick this to 9.x because I see #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 a change in pull request #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#discussion_r605994660
##########
File path: proxy/http/HttpSM.h
##########
@@ -166,6 +166,52 @@ enum HttpPluginTunnel_t {
class PluginVCCore;
+class PendingAction
+{
+public:
+ bool
+ operator==(Action *b)
+ {
+ return b == pending_action;
+ }
+ bool
+ operator!=(Action *b)
+ {
+ return b != pending_action;
+ }
+ PendingAction &
+ operator=(Action *b)
+ {
+ // Don't do anything if the new action is _DONE
+ if (b != ACTION_RESULT_DONE) {
+ if (b != pending_action && pending_action != nullptr) {
+ pending_action->cancel();
+ }
+ pending_action = b;
+ }
+ return *this;
+ }
+ Action *
+ operator->()
+ {
+ return pending_action;
+ }
+ Action *
+ get()
+ {
+ return pending_action;
+ }
+ ~PendingAction()
Review comment:
No - if this is ever used more normally (e.g. we stop using free lists) then the lack of a destructor will cause some nasty problems. We need to move toward more standard C++, not away from 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-814192613
[approve ci]
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#discussion_r607149849
##########
File path: proxy/http/HttpSM.h
##########
@@ -166,6 +166,52 @@ enum HttpPluginTunnel_t {
class PluginVCCore;
+class PendingAction
+{
+public:
+ bool
+ operator==(Action *b)
+ {
+ return b == pending_action;
+ }
+ bool
+ operator!=(Action *b)
+ {
+ return b != pending_action;
+ }
+ PendingAction &
+ operator=(Action *b)
+ {
+ // Don't do anything if the new action is _DONE
+ if (b != ACTION_RESULT_DONE) {
+ if (b != pending_action && pending_action != nullptr) {
+ pending_action->cancel();
+ }
+ pending_action = b;
+ }
+ return *this;
+ }
+ Action *
+ operator->()
+ {
+ return pending_action;
+ }
+ Action *
+ get()
+ {
+ return pending_action;
+ }
+ ~PendingAction()
Review comment:
Correct, but that's an artifact of the proxy allocators.
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-812000051
The null check was added to integrate more seamlessly in the existing code. Could consider adding a is_empty() or has_action() method instead or in addition.
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#discussion_r606040031
##########
File path: proxy/http/HttpSM.h
##########
@@ -166,6 +166,52 @@ enum HttpPluginTunnel_t {
class PluginVCCore;
+class PendingAction
+{
+public:
+ bool
+ operator==(Action *b)
+ {
+ return b == pending_action;
+ }
+ bool
+ operator!=(Action *b)
+ {
+ return b != pending_action;
+ }
+ PendingAction &
+ operator=(Action *b)
+ {
+ // Don't do anything if the new action is _DONE
+ if (b != ACTION_RESULT_DONE) {
+ if (b != pending_action && pending_action != nullptr) {
+ pending_action->cancel();
+ }
+ pending_action = b;
+ }
+ return *this;
+ }
+ Action *
+ operator->()
+ {
+ return pending_action;
+ }
+ Action *
+ get()
+ {
+ return pending_action;
+ }
+ ~PendingAction()
Review comment:
I'm not saying we don't need the destructor. My understanding is that this change is to ensure pending_action won't be triggered after HttpSM got unavailable. Manually canceling the action by assigning nullptr is fine. I just thought the current code doesn't fully take the benefit of PendingAction class.
--
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] vmamidi commented on pull request #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
vmamidi commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-812410036
[approve ci]
--
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 #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-812226485
I would say it is really a pointer wrapper, like `std::unique_ptr`. In fact, it might be interesting to see if it could be replaced with `std::unique_ptr` with a custom deleter.
--
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] ezelkow1 commented on pull request #7667: Add class to normalize handling of pending action
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #7667:
URL: https://github.com/apache/trafficserver/pull/7667#issuecomment-812759776
[approve ci]
--
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