You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "traeak (via GitHub)" <gi...@apache.org> on 2023/12/05 16:46:14 UTC
[PR] fix race condition with session thread migration [trafficserver]
traeak opened a new pull request, #10897:
URL: https://github.com/apache/trafficserver/pull/10897
This is a fix for a race condition when a session is taken from the session pool and migrated to another thread. There are still rare cases where events are still leaking through to the VC in the pre-migrated thread causing various different asserts.
--
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
Re: [PR] fix race condition with session thread migration [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #10897:
URL: https://github.com/apache/trafficserver/pull/10897#discussion_r1419475452
##########
src/proxy/http/HttpSessionManager.cc:
##########
@@ -440,57 +441,66 @@ HttpSessionManager::_acquire_session(sockaddr const *ip, CryptoHash const &hostn
} else {
retval = m_g_pool->acquireSession(ip, hostname_hash, match_style, sm, to_return);
Debug("http_ss", "[acquire session] global pool search %s", to_return ? "successful" : "failed");
- // At this point to_return has been removed from the pool. Do we need to move it
- // to the same thread?
- if (to_return) {
- UnixNetVConnection *server_vc = dynamic_cast<UnixNetVConnection *>(to_return->get_netvc());
- if (server_vc) {
- // Disable i/o on this vc now, but, hold onto the g_pool cont
- // and the mutex to stop any stray events from getting in
+ }
+
+ // At this point to_return has been removed from the pool.
+ if (nullptr != to_return) {
+ server_vc = dynamic_cast<UnixNetVConnection *>(to_return->get_netvc());
+ if (nullptr != server_vc) {
+ // Keep things from timing out on us
+ server_vc->set_inactivity_timeout(server_vc->get_inactivity_timeout());
+
+ // If thread must be migrated, clear out the VC's
+ // data and event handling on the original thread.
+ if (ethread != server_vc->get_thread()) {
+ SCOPED_MUTEX_LOCK(vclock, server_vc->mutex, ethread);
Review Comment:
just changing that to try-lock ends up with leaked connection messages in diags.log
--
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
Re: [PR] fix race condition with session thread migration [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #10897:
URL: https://github.com/apache/trafficserver/pull/10897#discussion_r1470387895
##########
src/proxy/http/HttpSessionManager.cc:
##########
@@ -151,52 +151,58 @@ ServerSessionPool::acquireSession(sockaddr const *addr, CryptoHash const &hostna
// This is broken out because only in this case do we check the host hash first. The range must be checked
// to verify an upstream that matches port and SNI name is selected. Walk backwards to select oldest.
in_port_t port = ats_ip_port_cast(addr);
- auto first = m_fqdn_pool.find(hostname_hash);
- while (first != m_fqdn_pool.end() && first->hostname_hash == hostname_hash) {
- Debug("http_ss", "Compare port 0x%x against 0x%x", port, ats_ip_port_cast(first->get_remote_addr()));
- if (port == ats_ip_port_cast(first->get_remote_addr()) &&
- (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_SNI) || validate_sni(sm, first->get_netvc())) &&
- (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTSNISYNC) || validate_host_sni(sm, first->get_netvc())) &&
- (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_CERT) || validate_cert(sm, first->get_netvc()))) {
- zret = HSM_DONE;
+ auto iter = m_fqdn_pool.find(hostname_hash);
Review Comment:
I noticed this iter variable is re-declared below. This is a nitpick, but it makes it harder to reason about the scope of this variable with the other one introduced.
--
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
Re: [PR] fix race condition with session thread migration [trafficserver]
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #10897:
URL: https://github.com/apache/trafficserver/pull/10897#discussion_r1419295053
##########
src/proxy/http/HttpSessionManager.cc:
##########
@@ -440,57 +441,66 @@ HttpSessionManager::_acquire_session(sockaddr const *ip, CryptoHash const &hostn
} else {
retval = m_g_pool->acquireSession(ip, hostname_hash, match_style, sm, to_return);
Debug("http_ss", "[acquire session] global pool search %s", to_return ? "successful" : "failed");
- // At this point to_return has been removed from the pool. Do we need to move it
- // to the same thread?
- if (to_return) {
- UnixNetVConnection *server_vc = dynamic_cast<UnixNetVConnection *>(to_return->get_netvc());
- if (server_vc) {
- // Disable i/o on this vc now, but, hold onto the g_pool cont
- // and the mutex to stop any stray events from getting in
+ }
+
+ // At this point to_return has been removed from the pool.
+ if (nullptr != to_return) {
+ server_vc = dynamic_cast<UnixNetVConnection *>(to_return->get_netvc());
+ if (nullptr != server_vc) {
+ // Keep things from timing out on us
+ server_vc->set_inactivity_timeout(server_vc->get_inactivity_timeout());
+
+ // If thread must be migrated, clear out the VC's
+ // data and event handling on the original thread.
+ if (ethread != server_vc->get_thread()) {
+ SCOPED_MUTEX_LOCK(vclock, server_vc->mutex, ethread);
Review Comment:
Do you want to do a try-lock instead? If the other thread handles a close before you get the lock here, things could disappear from underneath?
--
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
Re: [PR] fix race condition with session thread migration [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #10897:
URL: https://github.com/apache/trafficserver/pull/10897#issuecomment-1892831733
[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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix race condition with session thread migration [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #10897:
URL: https://github.com/apache/trafficserver/pull/10897#discussion_r1471270949
##########
src/proxy/http/HttpSessionManager.cc:
##########
@@ -151,52 +151,58 @@ ServerSessionPool::acquireSession(sockaddr const *addr, CryptoHash const &hostna
// This is broken out because only in this case do we check the host hash first. The range must be checked
// to verify an upstream that matches port and SNI name is selected. Walk backwards to select oldest.
in_port_t port = ats_ip_port_cast(addr);
- auto first = m_fqdn_pool.find(hostname_hash);
- while (first != m_fqdn_pool.end() && first->hostname_hash == hostname_hash) {
- Debug("http_ss", "Compare port 0x%x against 0x%x", port, ats_ip_port_cast(first->get_remote_addr()));
- if (port == ats_ip_port_cast(first->get_remote_addr()) &&
- (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_SNI) || validate_sni(sm, first->get_netvc())) &&
- (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTSNISYNC) || validate_host_sni(sm, first->get_netvc())) &&
- (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_CERT) || validate_cert(sm, first->get_netvc()))) {
- zret = HSM_DONE;
+ auto iter = m_fqdn_pool.find(hostname_hash);
Review Comment:
There are 2 different scopes, the iterators are different types (perhaps kill the auto and make it more explicit?). Generally I only renamed "first" to "iter", and then moved the confusing zret logic down to the bottom of the function.
The changes to this function don't contribute to the PR itself, it's just cleanup.
--
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
Re: [PR] fix race condition with session thread migration [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #10897:
URL: https://github.com/apache/trafficserver/pull/10897#discussion_r1471270949
##########
src/proxy/http/HttpSessionManager.cc:
##########
@@ -151,52 +151,58 @@ ServerSessionPool::acquireSession(sockaddr const *addr, CryptoHash const &hostna
// This is broken out because only in this case do we check the host hash first. The range must be checked
// to verify an upstream that matches port and SNI name is selected. Walk backwards to select oldest.
in_port_t port = ats_ip_port_cast(addr);
- auto first = m_fqdn_pool.find(hostname_hash);
- while (first != m_fqdn_pool.end() && first->hostname_hash == hostname_hash) {
- Debug("http_ss", "Compare port 0x%x against 0x%x", port, ats_ip_port_cast(first->get_remote_addr()));
- if (port == ats_ip_port_cast(first->get_remote_addr()) &&
- (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_SNI) || validate_sni(sm, first->get_netvc())) &&
- (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTSNISYNC) || validate_host_sni(sm, first->get_netvc())) &&
- (!(match_style & TS_SERVER_SESSION_SHARING_MATCH_MASK_CERT) || validate_cert(sm, first->get_netvc()))) {
- zret = HSM_DONE;
+ auto iter = m_fqdn_pool.find(hostname_hash);
Review Comment:
There are 2 different scopes, the iterators are different types (perhaps kill the auto and make it more explicit?). Generally I only renamed "first" to "iter", and then moved the confusing zret logic down to the bottom of the function.
--
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
Re: [PR] fix race condition with session thread migration [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak merged PR #10897:
URL: https://github.com/apache/trafficserver/pull/10897
--
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
Re: [PR] fix race condition with session thread migration [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #10897:
URL: https://github.com/apache/trafficserver/pull/10897#issuecomment-1849010286
Current proposed fix will drop 1-2 bytes from the content body instead of aborting.
--
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