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/17 21:07:03 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7849: Make HttpSM server reference a Transaction instead of a Session
shinrich opened a new pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849
This is derived as a subset of the H2 to origin PR #7622. This PR adds no support for H2 to origin, but it performs the restructuring so the HttpSM refers to the server connection via a ProxyTransaction instead of a PoolableSession.
This was requested by @maskit in hopes this work would assist them in discovering the cause of the server session leak described in issue #7471. This restructuring should simplify server connection leaks because the server_txn object stays around until HttpSM::kill_this() when server_txn->transaction_done() is called. If the server session should be returned to the origin pool, it will have previously been marked so. Otherwise the server session will be closed. Similarly earlier calls to do_io_close on the server transaction will be held until transaction_done() is called.
In the case of an origin connection failure and subsequent retry, server_txn->transaction_done() is called in HttpSM::do_http_server_open() before another connection attempt is made.
The PR also includes some stats I added to help debug a pool buildup issue I ran into debugging this PR against our 9.1.x build in master.
I also pulled into the following commits to make the move to our 9.1.x branch from master easier.
PR #6241 - but I see that just got picked into 9.1.x already
PR #7571 - I think that one is there now too.
PR #7584 - which also just got pulled into 9.1.x
PR #7600
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r633875104
##########
File path: proxy/http/Http1ServerSession.cc
##########
@@ -223,3 +189,71 @@ void
Http1ServerSession::start()
{
}
+
+bool
+Http1ServerSession::is_chunked_encoding_supported() const
+{
+ return true;
+}
+
+void
+Http1ServerSession ::release_transaction()
Review comment:
Called from transaction_done to adjust the released_transacations count and cause any pending close or server session pool release to occur.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634041563
##########
File path: proxy/http/HttpSessionManager.h
##########
@@ -67,6 +67,9 @@ class ServerSessionPool : public Continuation
static bool validate_host_sni(HttpSM *sm, NetVConnection *netvc);
static bool validate_sni(HttpSM *sm, NetVConnection *netvc);
static bool validate_cert(HttpSM *sm, NetVConnection *netvc);
+ void removeSession(PoolableSession *ssn);
Review comment:
Can you make `removeSession` and `addSession` private functions for now because H1ServerSession doesn't need to call them directly?
I know the new functions are used by Http2ServerSession, and we can discuss the detail later on H2-to-Origin PR, but having `removeSession` and `addSession` as public functions in addition to `acquireSession` and `releaseSession` doesn't make sense to me. The interface would be confusing. I think the both H1ServerSession and H2ServerSession should be handled by HttpSessionManager consistently - unnecessary processes should be skipped inside HttpSessionManager.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r635205996
##########
File path: proxy/ProxyTransaction.h
##########
@@ -85,6 +88,10 @@ class ProxyTransaction : public VConnection
// Returns true if there is a request body for this request
virtual bool has_request_body(int64_t content_length, bool is_chunked_set) const;
+ sockaddr const *get_remote_addr() const;
+
+ virtual HTTPVersion get_version(HTTPHdr &hdr) const;
Review comment:
I am not sure CONNECT makes my head hurt. I would think that only one nested method would be interpreted over the CONNECT method. And ATS would only interpret it if the CONNECT was made over a non-TLS connection. Otherwise ATS treats the body as a blind tunnel.
Will have to read the RFC tea leaves on this.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#issuecomment-853362646
[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] shinrich commented on a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r633872877
##########
File path: iocore/net/libinknet_stub.cc
##########
@@ -21,11 +21,11 @@
limitations under the License.
*/
-#include "HttpSessionManager.h"
+class EThread;
+class Continuation;
Review comment:
Needed to avoid pulling in unnecessary stuff to the unit test.
--
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 a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634727738
##########
File path: proxy/http/HttpSessionManager.cc
##########
@@ -165,9 +165,11 @@ ServerSessionPool::acquireSession(sockaddr const *addr, CryptoHash const &hostna
}
if (zret == HSM_DONE) {
to_return = first;
- HTTP_DECREMENT_DYN_STAT(http_pooled_server_connections_stat);
- m_fqdn_pool.erase(first);
- m_ip_pool.erase(to_return);
+ this->removeSession(to_return);
+ } else {
+ if (first != m_fqdn_pool.end()) {
Review comment:
Bike shedding, but for easier read, why not make this an } else if { ?
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634027832
##########
File path: proxy/http/HttpSessionManager.cc
##########
@@ -484,3 +489,50 @@ HttpSessionManager::release_session(PoolableSession *to_release)
return released_p ? HSM_DONE : HSM_RETRY;
}
+
+void
+ServerSessionPool::removeSession(PoolableSession *to_remove)
+{
+ EThread *ethread = this_ethread();
+ SCOPED_MUTEX_LOCK(lock, mutex, ethread);
+ char peer_ip[INET6_ADDRPORTSTRLEN];
+ if (is_debug_tag_set("http_ss")) {
+ ats_ip_nptop(to_remove->get_remote_addr(), peer_ip, sizeof(peer_ip));
+ Debug("http_ss", "Remove session %p %s m_fqdn_pool size=%" PRId64 " m_ip_pool_size=%" PRId64, to_remove, peer_ip,
+ m_fqdn_pool.count(), m_ip_pool.count());
+ }
+ m_fqdn_pool.erase(to_remove);
+ if (m_ip_pool.erase(to_remove)) {
+ HTTP_DECREMENT_DYN_STAT(http_pooled_server_connections_stat);
+ }
+ if (is_debug_tag_set("http_ss")) {
+ Debug("http_ss", "After Remove session %p m_fqdn_pool size=%" PRId64 " m_ip_pool_size=%" PRId64, to_remove, m_fqdn_pool.count(),
+ m_ip_pool.count());
+ }
+}
+
+void
+ServerSessionPool::testSession(PoolableSession *ss)
Review comment:
It seems like this functions is unnecessary. It's called by `Http2ServerSession::test_session()` on #7622, but `Http2ServerSession::test_session()` 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] zwoop commented on a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634471847
##########
File path: proxy/http/HttpSM.cc
##########
@@ -1800,6 +1798,65 @@ HttpSM::handle_api_return()
}
}
+void
+HttpSM::set_server_txn(ProxyTransaction *txn)
+{
+ ink_release_assert(server_txn == nullptr);
Review comment:
I would probably argue that this should be a debug assert, it kinda feels like we're too liberal sometimes with the release assert. Looking where this is called, it can't be nullptr (assuming that to_return->new_transaction() is not allowed to fail).
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r635202877
##########
File path: proxy/http/HttpSessionManager.cc
##########
@@ -484,3 +489,50 @@ HttpSessionManager::release_session(PoolableSession *to_release)
return released_p ? HSM_DONE : HSM_RETRY;
}
+
+void
+ServerSessionPool::removeSession(PoolableSession *to_remove)
+{
+ EThread *ethread = this_ethread();
+ SCOPED_MUTEX_LOCK(lock, mutex, ethread);
+ char peer_ip[INET6_ADDRPORTSTRLEN];
+ if (is_debug_tag_set("http_ss")) {
+ ats_ip_nptop(to_remove->get_remote_addr(), peer_ip, sizeof(peer_ip));
+ Debug("http_ss", "Remove session %p %s m_fqdn_pool size=%" PRId64 " m_ip_pool_size=%" PRId64, to_remove, peer_ip,
+ m_fqdn_pool.count(), m_ip_pool.count());
+ }
+ m_fqdn_pool.erase(to_remove);
+ if (m_ip_pool.erase(to_remove)) {
+ HTTP_DECREMENT_DYN_STAT(http_pooled_server_connections_stat);
+ }
+ if (is_debug_tag_set("http_ss")) {
+ Debug("http_ss", "After Remove session %p m_fqdn_pool size=%" PRId64 " m_ip_pool_size=%" PRId64, to_remove, m_fqdn_pool.count(),
+ m_ip_pool.count());
+ }
+}
+
+void
+ServerSessionPool::testSession(PoolableSession *ss)
Review comment:
Yes, this one can be removed.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r635203206
##########
File path: proxy/http/HttpSessionManager.h
##########
@@ -67,6 +67,9 @@ class ServerSessionPool : public Continuation
static bool validate_host_sni(HttpSM *sm, NetVConnection *netvc);
static bool validate_sni(HttpSM *sm, NetVConnection *netvc);
static bool validate_cert(HttpSM *sm, NetVConnection *netvc);
+ void removeSession(PoolableSession *ssn);
Review comment:
Sure, in this use case they can be made private
--
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 a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634480565
##########
File path: proxy/http/HttpSM.cc
##########
@@ -5095,11 +5123,26 @@ HttpSM::do_http_server_open(bool raw)
else if (ua_txn != nullptr) {
PoolableSession *existing_ss = ua_txn->get_server_session();
if (existing_ss) {
- existing_ss->get_netvc()->set_inactivity_timeout(HRTIME_SECONDS(t_state.txn_conf->keep_alive_no_activity_timeout_out));
- existing_ss->release(nullptr);
+ existing_ss->set_inactivity_timeout(HRTIME_SECONDS(t_state.txn_conf->keep_alive_no_activity_timeout_out));
+ existing_ss->release(server_txn);
ua_txn->attach_server_session(nullptr);
}
}
+
+ if (!try_reuse) {
+ HTTP_INCREMENT_DYN_STAT(http_origin_make_new);
+ if (TS_SERVER_SESSION_SHARING_MATCH_NONE == t_state.txn_conf->server_session_sharing_match) {
Review comment:
This code is slightly alarming; It feels like the server session would be in one of several conditions, as seen by which metric we increment. But, the logic to decide what "state" it is in is based on a number of conditions and member variables, rather than a single "state". That feels like e.g. someone could set "raw" and "is_private", and the session is now in a unknown state.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r639211955
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -221,6 +221,24 @@ ProxyTransaction::has_request_body(int64_t request_content_length, bool is_chunk
return request_content_length > 0 || is_chunked;
}
+bool
+ProxyTransaction::is_read_closed() const
Review comment:
Removed for now. Will be added back with the H2 to origin logic.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634115697
##########
File path: proxy/ProxyTransaction.h
##########
@@ -114,15 +121,17 @@ class ProxyTransaction : public VConnection
// This function must return a non-negative number that is different for two in-progress transactions with the same proxy_ssn
// session.
//
- void set_rx_error_code(ProxyError e);
- void set_tx_error_code(ProxyError e);
+ virtual void set_rx_error_code(ProxyError e);
+ virtual void set_tx_error_code(ProxyError e);
bool support_sni() const;
/// Variables
//
HttpSessionAccept::Options upstream_outbound_options; // overwritable copy of options
+ IOBufferReader *get_reader();
Review comment:
It's almost bike-shedding and this is not the only place, but I don't like this name very much because the data source (or the direction) is unclear. My understanding is that this reader always reads data from a peer (i.e. a remote host) regardless of the type of transaction (ClientTransaction or ServerTransaction).
How about `get_remote_data_reader` ?
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r633873300
##########
File path: proxy/PoolableSession.h
##########
@@ -192,3 +206,34 @@ PoolableSession::FQDNLinkage::equal(CryptoHash const &lhs, CryptoHash const &rhs
{
return lhs == rhs;
}
+
Review comment:
Promoted the conntecting tracking calls to a super 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] shinrich commented on a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r635211204
##########
File path: proxy/http/HttpSM.cc
##########
@@ -1800,6 +1798,65 @@ HttpSM::handle_api_return()
}
}
+void
+HttpSM::set_server_txn(ProxyTransaction *txn)
+{
+ ink_release_assert(server_txn == nullptr);
Review comment:
Agree. I use the release_asserts liberally while debugging in production. Don't always get them cleaned up. Looking forward to using your enable-all asserts option.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r635212369
##########
File path: proxy/http/HttpSM.cc
##########
@@ -5095,11 +5123,26 @@ HttpSM::do_http_server_open(bool raw)
else if (ua_txn != nullptr) {
PoolableSession *existing_ss = ua_txn->get_server_session();
if (existing_ss) {
- existing_ss->get_netvc()->set_inactivity_timeout(HRTIME_SECONDS(t_state.txn_conf->keep_alive_no_activity_timeout_out));
- existing_ss->release(nullptr);
+ existing_ss->set_inactivity_timeout(HRTIME_SECONDS(t_state.txn_conf->keep_alive_no_activity_timeout_out));
+ existing_ss->release(server_txn);
ua_txn->attach_server_session(nullptr);
}
}
+
+ if (!try_reuse) {
+ HTTP_INCREMENT_DYN_STAT(http_origin_make_new);
+ if (TS_SERVER_SESSION_SHARING_MATCH_NONE == t_state.txn_conf->server_session_sharing_match) {
Review comment:
I added these stats for debugging to solve my last issue before posting this PR. No doubt could be tidied up. Or removed entirely.
--
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 a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634418547
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -221,6 +221,24 @@ ProxyTransaction::has_request_body(int64_t request_content_length, bool is_chunk
return request_content_length > 0 || is_chunked;
}
+bool
+ProxyTransaction::is_read_closed() const
Review comment:
Is this used in some subsequent PRs? Seems a little odd to have this "no-op" method, which is never called as far as I can tell.
--
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 a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634480565
##########
File path: proxy/http/HttpSM.cc
##########
@@ -5095,11 +5123,26 @@ HttpSM::do_http_server_open(bool raw)
else if (ua_txn != nullptr) {
PoolableSession *existing_ss = ua_txn->get_server_session();
if (existing_ss) {
- existing_ss->get_netvc()->set_inactivity_timeout(HRTIME_SECONDS(t_state.txn_conf->keep_alive_no_activity_timeout_out));
- existing_ss->release(nullptr);
+ existing_ss->set_inactivity_timeout(HRTIME_SECONDS(t_state.txn_conf->keep_alive_no_activity_timeout_out));
+ existing_ss->release(server_txn);
ua_txn->attach_server_session(nullptr);
}
}
+
+ if (!try_reuse) {
+ HTTP_INCREMENT_DYN_STAT(http_origin_make_new);
+ if (TS_SERVER_SESSION_SHARING_MATCH_NONE == t_state.txn_conf->server_session_sharing_match) {
Review comment:
This code is slightly alarming; It feels like the server session would be in one of several conditions, as seen by which metric we increment. But, the logic to decide what "state" it is in is based on a number of conditions and member variables, rather than a single "state". That feels like e.g. someone could set "raw" and "is_private", and the behavior is now in a unknown state.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r633878368
##########
File path: proxy/http/HttpSM.cc
##########
@@ -3260,12 +3282,12 @@ HttpSM::is_bg_fill_necessary(HttpTunnelConsumer *c)
if (c->producer->alive && // something there to read
// server_entry && server_entry->vc && // from an origin server
- // server_session && server_session->get_netvc() && // which is still open and valid
+ // server_txn && server_txn->get_netvc() && // which is still open and valid
c->producer->num_consumers > 1 // with someone else reading it
) {
HttpTunnelProducer *p = nullptr;
- if (!server_entry || !server_entry->vc || !server_session || !server_session->get_netvc()) {
+ if (!server_txn || !server_txn->get_netvc()) {
Review comment:
May need to adjust this test, since server_txn will not be null at this point even if we did already call do_io_close on it. The get_netvc() may still be returning null, so it may still be ok.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634157355
##########
File path: proxy/http/HttpSM.cc
##########
@@ -1800,6 +1798,65 @@ HttpSM::handle_api_return()
}
}
+void
+HttpSM::set_server_txn(ProxyTransaction *txn)
+{
+ ink_release_assert(server_txn == nullptr);
+ server_txn = txn;
+ server_txn->attach_transaction(this);
+}
+
+PoolableSession *
+HttpSM::create_server_session(NetVConnection *netvc)
+{
+ HttpTransact::State &s = this->t_state;
+ PoolableSession *retval = httpServerSessionAllocator.alloc();
+
+ retval->sharing_pool = static_cast<TSServerSessionSharingPoolType>(s.http_config_param->server_session_sharing_pool);
+ retval->sharing_match = static_cast<TSServerSessionSharingMatchMask>(s.txn_conf->server_session_sharing_match);
+ MIOBuffer *netvc_read_buffer = new_MIOBuffer(HTTP_SERVER_RESP_HDR_BUFFER_INDEX);
+ IOBufferReader *netvc_reader = netvc_read_buffer->alloc_reader();
+ retval->new_connection(netvc, netvc_read_buffer, netvc_reader);
+
+ retval->attach_hostname(s.current.server->name);
+
+ ATS_PROBE1(new_origin_server_connection, s.current.server->name);
+ retval->set_active();
+
+ if (netvc) {
+ ats_ip_copy(&s.server_info.src_addr, netvc->get_local_addr());
+ }
+
+ // If origin_max_connections or origin_min_keep_alive_connections is set then we are metering
+ // the max and or min number of connections per host. Transfer responsibility for this to the
+ // session object.
+ if (s.outbound_conn_track_state.is_active()) {
+ Debug("http_connect", "[%" PRId64 "] max number of outbound connections: %d", this->sm_id, s.txn_conf->outbound_conntrack.max);
+ retval->enable_outbound_connection_tracking(s.outbound_conn_track_state.drop());
+ }
+ return retval;
+}
+
+void
+HttpSM::create_server_txn(NetVConnection *netvc, PoolableSession *new_session)
Review comment:
This function is really confusing.
The name implies that we call this function whenever we want to create a server transaction, but that is probably incorrect. This function seems to be only used for the initial server transaction on a session.
Furthermore, this is a public function despite that HttpSM has to be the only caller. Anybody can accidentally call this function based on the wrong assumption above.
Can you make this a private function and rename it to `create_initial_server_txn`? I haven't understood why server transactions have to be created here and there, and I think it should be done at only one place, but the name would tell that is the current design.
--
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 a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634432970
##########
File path: proxy/http/Http1ClientSession.cc
##########
@@ -345,11 +352,21 @@ Http1ClientSession::state_keep_alive(int event, void *data)
{
// Route the event. It is either for vc or
// the origin server slave vc
- if (data && data == slave_ka_vio) {
- return state_slave_keep_alive(event, data);
- } else {
- ink_assert(data && data == ka_vio);
- ink_assert(read_state == HCS_KEEP_ALIVE);
+ if (data) {
Review comment:
Would this possibly read nicer as a 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] shinrich commented on pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#issuecomment-842693995
[ci approve clang-analyzer]
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r635210244
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -221,6 +221,24 @@ ProxyTransaction::has_request_body(int64_t request_content_length, bool is_chunk
return request_content_length > 0 || is_chunked;
}
+bool
+ProxyTransaction::is_read_closed() const
Review comment:
Probably leftover from the H2 to origin pull back.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r635209778
##########
File path: proxy/http/HttpSM.cc
##########
@@ -1800,6 +1798,65 @@ HttpSM::handle_api_return()
}
}
+void
+HttpSM::set_server_txn(ProxyTransaction *txn)
+{
+ ink_release_assert(server_txn == nullptr);
+ server_txn = txn;
+ server_txn->attach_transaction(this);
+}
+
+PoolableSession *
+HttpSM::create_server_session(NetVConnection *netvc)
+{
+ HttpTransact::State &s = this->t_state;
+ PoolableSession *retval = httpServerSessionAllocator.alloc();
+
+ retval->sharing_pool = static_cast<TSServerSessionSharingPoolType>(s.http_config_param->server_session_sharing_pool);
+ retval->sharing_match = static_cast<TSServerSessionSharingMatchMask>(s.txn_conf->server_session_sharing_match);
+ MIOBuffer *netvc_read_buffer = new_MIOBuffer(HTTP_SERVER_RESP_HDR_BUFFER_INDEX);
+ IOBufferReader *netvc_reader = netvc_read_buffer->alloc_reader();
+ retval->new_connection(netvc, netvc_read_buffer, netvc_reader);
+
+ retval->attach_hostname(s.current.server->name);
+
+ ATS_PROBE1(new_origin_server_connection, s.current.server->name);
+ retval->set_active();
+
+ if (netvc) {
+ ats_ip_copy(&s.server_info.src_addr, netvc->get_local_addr());
+ }
+
+ // If origin_max_connections or origin_min_keep_alive_connections is set then we are metering
+ // the max and or min number of connections per host. Transfer responsibility for this to the
+ // session object.
+ if (s.outbound_conn_track_state.is_active()) {
+ Debug("http_connect", "[%" PRId64 "] max number of outbound connections: %d", this->sm_id, s.txn_conf->outbound_conntrack.max);
+ retval->enable_outbound_connection_tracking(s.outbound_conn_track_state.drop());
+ }
+ return retval;
+}
+
+void
+HttpSM::create_server_txn(NetVConnection *netvc, PoolableSession *new_session)
Review comment:
create_initial_server_txn is probably a better name. And it can be made private. The goal of the function is to wire in the netvc into a ProxyTransaction object (Http1ServerTransaction in this case). It is taking logic that was inline in the HttpSM::state_http_server_open function. It is called a bit later once H2 to origin is introduced because we need to wait to see what protocol is negotiated to create the correct subclass of ProxyTransaction.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r639209287
##########
File path: proxy/http/HttpSM.cc
##########
@@ -1800,6 +1798,65 @@ HttpSM::handle_api_return()
}
}
+void
+HttpSM::set_server_txn(ProxyTransaction *txn)
+{
+ ink_release_assert(server_txn == nullptr);
+ server_txn = txn;
+ server_txn->attach_transaction(this);
+}
+
+PoolableSession *
+HttpSM::create_server_session(NetVConnection *netvc)
+{
+ HttpTransact::State &s = this->t_state;
+ PoolableSession *retval = httpServerSessionAllocator.alloc();
+
+ retval->sharing_pool = static_cast<TSServerSessionSharingPoolType>(s.http_config_param->server_session_sharing_pool);
+ retval->sharing_match = static_cast<TSServerSessionSharingMatchMask>(s.txn_conf->server_session_sharing_match);
+ MIOBuffer *netvc_read_buffer = new_MIOBuffer(HTTP_SERVER_RESP_HDR_BUFFER_INDEX);
+ IOBufferReader *netvc_reader = netvc_read_buffer->alloc_reader();
+ retval->new_connection(netvc, netvc_read_buffer, netvc_reader);
+
+ retval->attach_hostname(s.current.server->name);
+
+ ATS_PROBE1(new_origin_server_connection, s.current.server->name);
+ retval->set_active();
+
+ if (netvc) {
+ ats_ip_copy(&s.server_info.src_addr, netvc->get_local_addr());
+ }
+
+ // If origin_max_connections or origin_min_keep_alive_connections is set then we are metering
+ // the max and or min number of connections per host. Transfer responsibility for this to the
+ // session object.
+ if (s.outbound_conn_track_state.is_active()) {
+ Debug("http_connect", "[%" PRId64 "] max number of outbound connections: %d", this->sm_id, s.txn_conf->outbound_conntrack.max);
+ retval->enable_outbound_connection_tracking(s.outbound_conn_track_state.drop());
+ }
+ return retval;
+}
+
+void
+HttpSM::create_server_txn(NetVConnection *netvc, PoolableSession *new_session)
Review comment:
Reworked so create_server_txn is called in HttpSessionManager too instead of calling new_transaction directly
--
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 removed a comment on pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich removed a comment on pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#issuecomment-842693995
[ci approve clang-analyzer]
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#issuecomment-842672989
[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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634881682
##########
File path: proxy/http/Http1ServerSession.cc
##########
@@ -93,26 +98,19 @@ Http1ServerSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
}
void
-Http1ServerSession::enable_outbound_connection_tracking(OutboundConnTrack::Group *group)
+Http1ServerSession::do_io_close(int alerrno)
{
- ink_assert(nullptr == conn_track_group);
- conn_track_group = group;
- if (is_debug_tag_set("http_ss")) {
- ts::LocalBufferWriter<256> w;
- w.print("[{}] new connection, ip: {}, group ({}), count: {}\0", con_id, get_server_ip(), *group, group->_count);
- Debug("http_ss", "%s", w.data());
+ if (state == SSN_CLOSED) { // Already been closed
Review comment:
I prefer this way. It has less dup code.
```
if (state != SSN_CLOSED) {
state = SSN_CLOSED
release outbound connection tracking
close vc
decrement stats
}
if (transact_count == released_transactions) {
this->destroy();
}
```
--
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 a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634468886
##########
File path: proxy/http/HttpConfig.cc
##########
@@ -393,6 +393,25 @@ register_stat_callbacks()
RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin_shutdown.tunnel_abort", RECD_INT, RECP_NON_PERSISTENT,
(int)http_origin_shutdown_tunnel_abort, RecRawStatSyncCount);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin.reuse", RECD_INT, RECP_NON_PERSISTENT,
+ (int)http_origin_reuse, RecRawStatSyncCount);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin.not_found", RECD_INT, RECP_NON_PERSISTENT,
+ (int)http_origin_not_found, RecRawStatSyncCount);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin.reuse_fail", RECD_INT, RECP_NON_PERSISTENT,
+ (int)http_origin_reuse_fail, RecRawStatSyncCount);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin.make_new", RECD_INT, RECP_NON_PERSISTENT,
+ (int)http_origin_make_new, RecRawStatSyncCount);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin.no_sharing", RECD_INT, RECP_NON_PERSISTENT,
+ (int)http_origin_no_sharing, RecRawStatSyncCount);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin.body", RECD_INT, RECP_NON_PERSISTENT, (int)http_origin_body,
+ RecRawStatSyncCount);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin.private", RECD_INT, RECP_NON_PERSISTENT,
+ (int)http_origin_private, RecRawStatSyncCount);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin.close_private", RECD_INT, RECP_NON_PERSISTENT,
+ (int)http_origin_close_private, RecRawStatSyncCount);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.origin.raw", RECD_INT, RECP_NON_PERSISTENT, (int)http_origin_raw,
+ RecRawStatSyncCount);
+
Review comment:
This is great!
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634094331
##########
File path: proxy/ProxyTransaction.h
##########
@@ -85,6 +88,10 @@ class ProxyTransaction : public VConnection
// Returns true if there is a request body for this request
virtual bool has_request_body(int64_t content_length, bool is_chunked_set) const;
+ sockaddr const *get_remote_addr() const;
+
+ virtual HTTPVersion get_version(HTTPHdr &hdr) const;
Review comment:
We don't need this at the moment.
Since`ProxySession::get_version` receives `HTTPHdr`, a proxy session can always return correct version. I wonder if there will be a session that has multiple versions of transactions. I'm not sure if we are allowed to use multiple versions on a traditional HTTP connection like below:
```
[Connect]
GET /a HTTP/1.0
GET /b HTTP/1.1
Host: example.com
[Close]
```
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r633986178
##########
File path: proxy/http/HttpSessionManager.cc
##########
@@ -484,3 +489,50 @@ HttpSessionManager::release_session(PoolableSession *to_release)
return released_p ? HSM_DONE : HSM_RETRY;
}
+
+void
+ServerSessionPool::removeSession(PoolableSession *to_remove)
+{
+ EThread *ethread = this_ethread();
+ SCOPED_MUTEX_LOCK(lock, mutex, ethread);
+ char peer_ip[INET6_ADDRPORTSTRLEN];
+ if (is_debug_tag_set("http_ss")) {
+ ats_ip_nptop(to_remove->get_remote_addr(), peer_ip, sizeof(peer_ip));
+ Debug("http_ss", "Remove session %p %s m_fqdn_pool size=%" PRId64 " m_ip_pool_size=%" PRId64, to_remove, peer_ip,
Review comment:
Please use `%zu` for `size_t`. https://github.com/apache/trafficserver/pull/7830
```
HttpSessionManager.cc:502:11: error: format specifies type 'long long' but the argument has type 'size_t' (aka 'unsigned long') [-Werror,-Wformat]
m_fqdn_pool.count(), m_ip_pool.count());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../include/tscore/Diags.h:158:35: note: expanded from macro 'Debug'
log_message.debug(tag, loc, __VA_ARGS__); \
^~~~~~~~~~~
HttpSessionManager.cc:502:32: error: format specifies type 'long long' but the argument has type 'size_t' (aka 'unsigned long') [-Werror,-Wformat]
m_fqdn_pool.count(), m_ip_pool.count());
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
../../include/tscore/Diags.h:158:35: note: expanded from macro 'Debug'
log_message.debug(tag, loc, __VA_ARGS__); \
^~~~~~~~~~~
HttpSessionManager.cc:509:113: error: format specifies type 'long long' but the argument has type 'size_t' (aka 'unsigned long') [-Werror,-Wformat]
Debug("http_ss", "After Remove session %p m_fqdn_pool size=%" PRId64 " m_ip_pool_size=%" PRId64, to_remove, m_fqdn_pool.count(),
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
../../include/tscore/Diags.h:158:35: note: expanded from macro 'Debug'
log_message.debug(tag, loc, __VA_ARGS__); \
^~~~~~~~~~~
HttpSessionManager.cc:510:11: error: format specifies type 'long long' but the argument has type 'size_t' (aka 'unsigned long') [-Werror,-Wformat]
m_ip_pool.count());
^~~~~~~~~~~~~~~~~~
../../include/tscore/Diags.h:158:35: note: expanded from macro 'Debug'
log_message.debug(tag, loc, __VA_ARGS__); \
^~~~~~~~~~~
4 errors generated.
```
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r639214178
##########
File path: proxy/http/Http1ClientSession.cc
##########
@@ -345,11 +352,21 @@ Http1ClientSession::state_keep_alive(int event, void *data)
{
// Route the event. It is either for vc or
// the origin server slave vc
- if (data && data == slave_ka_vio) {
- return state_slave_keep_alive(event, data);
- } else {
- ink_assert(data && data == ka_vio);
- ink_assert(read_state == HCS_KEEP_ALIVE);
+ if (data) {
Review comment:
Cannot have variables as case elements.
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r635206949
##########
File path: proxy/ProxyTransaction.h
##########
@@ -114,15 +121,17 @@ class ProxyTransaction : public VConnection
// This function must return a non-negative number that is different for two in-progress transactions with the same proxy_ssn
// session.
//
- void set_rx_error_code(ProxyError e);
- void set_tx_error_code(ProxyError e);
+ virtual void set_rx_error_code(ProxyError e);
+ virtual void set_tx_error_code(ProxyError e);
bool support_sni() const;
/// Variables
//
HttpSessionAccept::Options upstream_outbound_options; // overwritable copy of options
+ IOBufferReader *get_reader();
Review comment:
I'm ok with get_remote_reader or remote_data_reader. Just added this as a wrapper method around the _reader member.
--
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 a change in pull request #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#discussion_r634726557
##########
File path: proxy/http/HttpSM.cc
##########
@@ -5492,14 +5534,13 @@ HttpSM::release_server_session(bool serve_from_cache)
} else {
HTTP_INCREMENT_DYN_STAT(http_origin_shutdown_release_misc);
}
- server_session->do_io_close();
}
- ink_assert(server_entry->vc == server_session);
- server_entry->in_tunnel = true;
- vc_table.cleanup_entry(server_entry);
- server_entry = nullptr;
- server_session = nullptr;
+ if (server_entry) {
+ server_entry->vc = nullptr;
+ server_entry->read_vio = server_entry->write_vio = nullptr;
Review comment:
This would look a lot better if you don't do the double assignment on line 5541 :).
--
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 #7849: Make HttpSM server reference a Transaction instead of a Session
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7849:
URL: https://github.com/apache/trafficserver/pull/7849#issuecomment-842694244
[approve ci clang-analyzer]
--
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