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/06/02 20:20:32 UTC
[GitHub] [trafficserver] bneradt opened a new pull request #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
bneradt opened a new pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913
This causes the VCONN_CLOSE hooks to be called more consistently in the
event of TS_EVENT_VCONN_CLOSE. Without this patch it got missed under
some circumstances, causing situations where plugins would receive a
TS_EVENT_VCONN_START but never a TS_EVENT_VCONN_CLOSE for some
connections.
--
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 merged pull request #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
bneradt merged pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913
--
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 #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#issuecomment-853869105
Looks good!
--
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 a change in pull request #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#discussion_r645131456
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -1694,6 +1694,12 @@ SSLNetVConnection::callHooks(TSEvent eventId)
Debug("ssl", "sslHandshakeHookState=%d eventID=%d", this->sslHandshakeHookState, eventId);
// Move state if it is appropriate
+ if (eventId == TS_EVENT_VCONN_CLOSE) {
+ // Regardless of state, if the connection is closing, then transition to
+ // the DONE state. This will trigger us to call the appropriate cleanup
+ // routines.
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
+ }
Review comment:
Good idea.
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -1694,39 +1694,40 @@ SSLNetVConnection::callHooks(TSEvent eventId)
Debug("ssl", "sslHandshakeHookState=%d eventID=%d", this->sslHandshakeHookState, eventId);
// Move state if it is appropriate
- switch (this->sslHandshakeHookState) {
- case HANDSHAKE_HOOKS_PRE:
- case HANDSHAKE_HOOKS_OUTBOUND_PRE:
- if (eventId == TS_EVENT_SSL_CLIENT_HELLO) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_CLIENT_HELLO;
- } else if (eventId == TS_EVENT_SSL_SERVERNAME) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_SNI;
- } else if (eventId == TS_EVENT_SSL_VERIFY_SERVER) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_VERIFY_SERVER;
- } else if (eventId == TS_EVENT_SSL_CERT) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
- }
- break;
- case HANDSHAKE_HOOKS_CLIENT_HELLO:
- if (eventId == TS_EVENT_SSL_SERVERNAME) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_SNI;
- } else if (eventId == TS_EVENT_SSL_CERT) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
- } else if (eventId == TS_EVENT_VCONN_CLOSE) {
- // Jump to the end
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
- }
- break;
- case HANDSHAKE_HOOKS_SNI:
- if (eventId == TS_EVENT_SSL_CERT) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
- } else if (eventId == TS_EVENT_VCONN_CLOSE) {
- // Jump to the end
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
+ if (eventId == TS_EVENT_VCONN_CLOSE) {
+ // Regardless of state, if the connection is closing, then transition to
+ // the DONE state. This will trigger us to call the appropriate cleanup
+ // routines.
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
+ } else {
+ switch (this->sslHandshakeHookState) {
+ case HANDSHAKE_HOOKS_PRE:
+ case HANDSHAKE_HOOKS_OUTBOUND_PRE:
+ if (eventId == TS_EVENT_SSL_CLIENT_HELLO) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_CLIENT_HELLO;
+ } else if (eventId == TS_EVENT_SSL_SERVERNAME) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_SNI;
+ } else if (eventId == TS_EVENT_SSL_VERIFY_SERVER) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_VERIFY_SERVER;
+ } else if (eventId == TS_EVENT_SSL_CERT) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
+ }
+ break;
+ case HANDSHAKE_HOOKS_CLIENT_HELLO:
+ if (eventId == TS_EVENT_SSL_SERVERNAME) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_SNI;
+ } else if (eventId == TS_EVENT_SSL_CERT) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
+ }
+ break;
+ case HANDSHAKE_HOOKS_SNI:
+ if (eventId == TS_EVENT_SSL_CERT) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
+ }
+ break;
+ default:
+ break;
}
- break;
- default:
- break;
}
Review comment:
View this patch using the "Hide whitespace changes" option via github.
--
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 #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#issuecomment-853388436
[approve ci autest]
--
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 #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#issuecomment-853388436
[approve ci autest]
--
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 #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#discussion_r644394579
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -1694,6 +1694,12 @@ SSLNetVConnection::callHooks(TSEvent eventId)
Debug("ssl", "sslHandshakeHookState=%d eventID=%d", this->sslHandshakeHookState, eventId);
// Move state if it is appropriate
+ if (eventId == TS_EVENT_VCONN_CLOSE) {
+ // Regardless of state, if the connection is closing, then transition to
+ // the DONE state. This will trigger us to call the appropriate cleanup
+ // routines.
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
+ }
Review comment:
Looks reasonable. We may want to put the `switch` statement below into `else` clause to ensure `sslHandshakeHookState` won't be accidentally overwritten in the `switch` 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 #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#issuecomment-908722586
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#issuecomment-908722586
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.
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 removed a comment on pull request #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#issuecomment-853388436
[approve ci autest]
--
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 a change in pull request #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#discussion_r645131456
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -1694,6 +1694,12 @@ SSLNetVConnection::callHooks(TSEvent eventId)
Debug("ssl", "sslHandshakeHookState=%d eventID=%d", this->sslHandshakeHookState, eventId);
// Move state if it is appropriate
+ if (eventId == TS_EVENT_VCONN_CLOSE) {
+ // Regardless of state, if the connection is closing, then transition to
+ // the DONE state. This will trigger us to call the appropriate cleanup
+ // routines.
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
+ }
Review comment:
Good idea.
--
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 a change in pull request #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#discussion_r645149183
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -1694,39 +1694,40 @@ SSLNetVConnection::callHooks(TSEvent eventId)
Debug("ssl", "sslHandshakeHookState=%d eventID=%d", this->sslHandshakeHookState, eventId);
// Move state if it is appropriate
- switch (this->sslHandshakeHookState) {
- case HANDSHAKE_HOOKS_PRE:
- case HANDSHAKE_HOOKS_OUTBOUND_PRE:
- if (eventId == TS_EVENT_SSL_CLIENT_HELLO) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_CLIENT_HELLO;
- } else if (eventId == TS_EVENT_SSL_SERVERNAME) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_SNI;
- } else if (eventId == TS_EVENT_SSL_VERIFY_SERVER) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_VERIFY_SERVER;
- } else if (eventId == TS_EVENT_SSL_CERT) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
- }
- break;
- case HANDSHAKE_HOOKS_CLIENT_HELLO:
- if (eventId == TS_EVENT_SSL_SERVERNAME) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_SNI;
- } else if (eventId == TS_EVENT_SSL_CERT) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
- } else if (eventId == TS_EVENT_VCONN_CLOSE) {
- // Jump to the end
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
- }
- break;
- case HANDSHAKE_HOOKS_SNI:
- if (eventId == TS_EVENT_SSL_CERT) {
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
- } else if (eventId == TS_EVENT_VCONN_CLOSE) {
- // Jump to the end
- this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
+ if (eventId == TS_EVENT_VCONN_CLOSE) {
+ // Regardless of state, if the connection is closing, then transition to
+ // the DONE state. This will trigger us to call the appropriate cleanup
+ // routines.
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
+ } else {
+ switch (this->sslHandshakeHookState) {
+ case HANDSHAKE_HOOKS_PRE:
+ case HANDSHAKE_HOOKS_OUTBOUND_PRE:
+ if (eventId == TS_EVENT_SSL_CLIENT_HELLO) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_CLIENT_HELLO;
+ } else if (eventId == TS_EVENT_SSL_SERVERNAME) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_SNI;
+ } else if (eventId == TS_EVENT_SSL_VERIFY_SERVER) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_VERIFY_SERVER;
+ } else if (eventId == TS_EVENT_SSL_CERT) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
+ }
+ break;
+ case HANDSHAKE_HOOKS_CLIENT_HELLO:
+ if (eventId == TS_EVENT_SSL_SERVERNAME) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_SNI;
+ } else if (eventId == TS_EVENT_SSL_CERT) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
+ }
+ break;
+ case HANDSHAKE_HOOKS_SNI:
+ if (eventId == TS_EVENT_SSL_CERT) {
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT;
+ }
+ break;
+ default:
+ break;
}
- break;
- default:
- break;
}
Review comment:
View this patch using the "Hide whitespace changes" option via github.
--
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 #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#issuecomment-904126748
Marking this for the 9.1.x train. If plugins rely upon the VCONN_CLOSE hook to free memory, this will result in a memory leak. Or any host of other unexpected behaviors when the hook isn't called when expected.
--
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] shinrich commented on pull request #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#issuecomment-853869105
Looks good!
--
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 #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#discussion_r644394579
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -1694,6 +1694,12 @@ SSLNetVConnection::callHooks(TSEvent eventId)
Debug("ssl", "sslHandshakeHookState=%d eventID=%d", this->sslHandshakeHookState, eventId);
// Move state if it is appropriate
+ if (eventId == TS_EVENT_VCONN_CLOSE) {
+ // Regardless of state, if the connection is closing, then transition to
+ // the DONE state. This will trigger us to call the appropriate cleanup
+ // routines.
+ this->sslHandshakeHookState = HANDSHAKE_HOOKS_DONE;
+ }
Review comment:
Looks reasonable. We may want to put the `switch` statement below into `else` clause to ensure `sslHandshakeHookState` won't be accidentally overwritten in the `switch` 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] bneradt commented on pull request #7913: Ensure TS_VCONN_CLOSE_HOOK hook is called during TS_EVENT_VCONN_CLOSE.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7913:
URL: https://github.com/apache/trafficserver/pull/7913#issuecomment-853388436
[approve ci autest]
--
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