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/02 08:42:04 UTC
[GitHub] [trafficserver] maskit opened a new pull request #7571: Tidy up session/transaction destruction process
maskit opened a new pull request #7571:
URL: https://github.com/apache/trafficserver/pull/7571
The primary goal is to call destructors of ProxySession, ProxyTransaction and their subclasses.
ProxySession
Made `free()` pure virtual function -- ProxySession does not know how to free itself and it has never done it.
Add destructor -- The code originally in `free()` is now in the destructor.
ProxyTransaction
Remove destroy -- I don't see any reasons to have this as a normal function. The existence is confusing.
Add destructor -- The code originally in `destroy()` is now in the destructor
Http1ClientSession
Add destructor -- To call ProxySession's destructor automatically. The normal destruction process is easier to follow.
Call `Http1ClientTransaction::reset()` -- Originally `destroy()` was called to reuse transaction object. See Http1Transaction.
Http1ServerSession
Add destructor -- Just for consistency.
Add `free()` -- It does nothing because it's freed in `destroy()` unlike Http1ClientSession.
Http1Transaction
Rename `destroy()` to `reset()` -- `Http1Transaction::destroy()` didn't actually destroy itself, but reset its state.
Http2ClientSession
Add destructor -- To call ProxySession's destructor automatically.
Http2Stream
Convert `destroy()` to the destructor
Unify the process to destroy itself
Http3Session
Add `free()` -- It has never been freed.
----------------------------------------------------------------
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 merged pull request #7571: Tidy up session/transaction destruction process
Posted by GitBox <gi...@apache.org>.
maskit merged pull request #7571:
URL: https://github.com/apache/trafficserver/pull/7571
----------------------------------------------------------------
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] ywkaras commented on a change in pull request #7571: Tidy up session/transaction destruction process
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7571:
URL: https://github.com/apache/trafficserver/pull/7571#discussion_r588541280
##########
File path: proxy/http/Http1ClientSession.h
##########
@@ -54,6 +54,7 @@ class Http1ClientSession : public ProxySession
public:
typedef ProxySession super; ///< Parent type.
Http1ClientSession();
+ ~Http1ClientSession() = default;
Review comment:
This isn't necessary.
----------------------------------------------------------------
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] ywkaras commented on pull request #7571: Tidy up session/transaction destruction process
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7571:
URL: https://github.com/apache/trafficserver/pull/7571#issuecomment-790888587
Looks like you forgot to set the Destruct_of_free parameter to the ClassAllocator instantiations to true.
----------------------------------------------------------------
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 #7571: Tidy up session/transaction destruction process
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7571:
URL: https://github.com/apache/trafficserver/pull/7571#issuecomment-791064677
> Looks like you forgot to set the Destruct_of_free parameter to the ClassAllocator instantiations to true.
I know you added the flag, but the PR was not merged yet when I made this change. And I don't think we should use the flag on this PR, because that would unnecessarily introduces a dependency for your change. Do you mind if I make the change separately on another PR? It would allows us to backport or revert our changes separately.
----------------------------------------------------------------
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 #7571: Tidy up session/transaction destruction process
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7571:
URL: https://github.com/apache/trafficserver/pull/7571#issuecomment-809665651
Need this for 9.1.x for other PRs to work.
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 #7571: Tidy up session/transaction destruction process
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7571:
URL: https://github.com/apache/trafficserver/pull/7571#issuecomment-789384793
This depends on #7567.
----------------------------------------------------------------
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 #7571: Tidy up session/transaction destruction process
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7571:
URL: https://github.com/apache/trafficserver/pull/7571#issuecomment-789384003
[approve ci clang-format]
----------------------------------------------------------------
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