Make HttpSM server reference a Transaction instead of a Session #7849

shinrich · 2021-05-17T21:06:48Z

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

shinrich · 2021-05-17T21:13:03Z

iocore/net/libinknet_stub.cc


-#include "HttpSessionManager.h"
+class EThread;
+class Continuation;


Needed to avoid pulling in unnecessary stuff to the unit test.

shinrich · 2021-05-17T21:13:52Z

proxy/PoolableSession.h

 {
  return lhs == rhs;
 }
+


Promoted the conntecting tracking calls to a super class.

shinrich · 2021-05-17T21:17:21Z

proxy/http/Http1ServerSession.cc

+}
+
+void
+Http1ServerSession ::release_transaction()


Called from transaction_done to adjust the released_transacations count and cause any pending close or server session pool release to occur.

shinrich · 2021-05-17T21:23:37Z

proxy/http/HttpSM.cc

    HttpTunnelProducer *p = nullptr;

-    if (!server_entry || !server_entry->vc || !server_session || !server_session->get_netvc()) {
+    if (!server_txn || !server_txn->get_netvc()) {


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.

shinrich · 2021-05-17T22:05:33Z

[approve ci autest]

shinrich · 2021-05-17T22:55:40Z

[approve ci clang-analyzer]

proxy/http/HttpSessionManager.cc

maskit · 2021-05-18T04:20:34Z

proxy/http/HttpSessionManager.cc

+}
+
+void
+ServerSessionPool::testSession(PoolableSession *ss)


It seems like this functions is unnecessary. It's called by Http2ServerSession::test_session() on #7622, but Http2ServerSession::test_session() is not called.

Yes, this one can be removed.

proxy/http/HttpSessionManager.h

maskit · 2021-05-18T06:53:14Z

proxy/ProxyTransaction.h


+  sockaddr const *get_remote_addr() const;
+
+  virtual HTTPVersion get_version(HTTPHdr &hdr) const;


We don't need this at the moment.

SinceProxySession::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]

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.

proxy/ProxyTransaction.h

maskit · 2021-05-18T08:24:28Z

proxy/http/HttpSM.cc

+}
+
+void
+HttpSM::create_server_txn(NetVConnection *netvc, PoolableSession *new_session)


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.

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.

Reworked so create_server_txn is called in HttpSessionManager too instead of calling new_transaction directly

zwoop · 2021-05-18T14:07:01Z

proxy/ProxyTransaction.cc

 }

+bool
+ProxyTransaction::is_read_closed() const


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.

Probably leftover from the H2 to origin pull back.

Removed for now. Will be added back with the H2 to origin logic.

zwoop · 2021-05-18T14:22:17Z

proxy/http/Http1ClientSession.cc

-  } else {
-    ink_assert(data && data == ka_vio);
-    ink_assert(read_state == HCS_KEEP_ALIVE);
+  if (data) {


Would this possibly read nicer as a switch statement?

Cannot have variables as case elements.

zwoop · 2021-05-18T15:00:06Z

proxy/http/HttpConfig.cc

+                     (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);
+


This is great!

zwoop · 2021-05-18T15:03:12Z

proxy/http/HttpSM.cc

+void
+HttpSM::set_server_txn(ProxyTransaction *txn)
+{
+  ink_release_assert(server_txn == nullptr);


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).

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.

zwoop · 2021-05-18T15:12:01Z

proxy/http/HttpSM.cc

+
+  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) {


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.

I added these stats for debugging to solve my last issue before posting this PR. No doubt could be tidied up. Or removed entirely.

zwoop · 2021-05-18T20:32:34Z

proxy/http/HttpSM.cc

-  server_session = nullptr;
+  if (server_entry) {
+    server_entry->vc       = nullptr;
+    server_entry->read_vio = server_entry->write_vio = nullptr;


This would look a lot better if you don't do the double assignment on line 5541 :).

zwoop · 2021-05-18T20:34:38Z

proxy/http/HttpSessionManager.cc

-      m_ip_pool.erase(to_return);
+      this->removeSession(to_return);
+    } else {
+      if (first != m_fqdn_pool.end()) {


Bike shedding, but for easier read, why not make this an } else if { ?

maskit · 2021-05-19T03:07:51Z

proxy/http/Http1ServerSession.cc

-    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


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(); }

maskit

Although I can't say this doesn't break things, but it looks good to me. I'm going to approve this when conflicts are resolved.

shinrich · 2021-06-02T20:30:57Z

[approve ci autest]

shinrich added the Origin Session label May 17, 2021

shinrich added this to the 10.0.0 milestone May 17, 2021

shinrich requested review from a-canary, maskit and vmamidi May 17, 2021 21:06

shinrich self-assigned this May 17, 2021

shinrich requested review from SolidWallOfCode, bryancall, masaori335 and zwoop as code owners May 17, 2021 21:06

shinrich commented May 17, 2021

View reviewed changes

proxy/PoolableSession.h

{

return lhs == rhs;

}

Copy link

Member Author

shinrich May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promoted the conntecting tracking calls to a super class.

shinrich commented May 17, 2021

View reviewed changes

maskit reviewed May 18, 2021

View reviewed changes

proxy/http/HttpSessionManager.cc Outdated Show resolved Hide resolved

maskit reviewed May 18, 2021

View reviewed changes

proxy/http/HttpSessionManager.h Outdated Show resolved Hide resolved

maskit reviewed May 18, 2021

View reviewed changes

proxy/ProxyTransaction.h Outdated Show resolved Hide resolved

maskit reviewed May 18, 2021

View reviewed changes

zwoop reviewed May 18, 2021

View reviewed changes

maskit reviewed May 19, 2021

View reviewed changes

maskit reviewed Jun 2, 2021

View reviewed changes

shinrich added 6 commits June 2, 2021 14:24

Make HttpSM server reference a Transaction instead of a Session

e52a987

Remove the intrusive hash map change

a3c65e5

Fix missing override

e986eb2

Make http3 happy

bea41a5

make clang-analyzer happy

19d2e37

Incorproate comments

7e9a61c

shinrich force-pushed the h1-origin-txn-restructure branch from 2e5a4ed to 7e9a61c Compare June 2, 2021 14:36

maskit approved these changes Jun 2, 2021

View reviewed changes

shinrich merged commit f839a74 into apache:master Jun 3, 2021

shinrich mentioned this pull request Jun 3, 2021

Fix race condition on server session state #7921

Merged

bneradt mentioned this pull request Jun 16, 2021

Intermittent failure on AuTest (double) #7946

Closed

shinrich mentioned this pull request Jun 18, 2021

Fix double test flakiness due to EOS/TXN_CLOSE race #7956

Merged

shinrich mentioned this pull request Jun 30, 2021

Fix use after free #8022

Merged

shinrich mentioned this pull request Jul 19, 2021

Assert due to active timeout on server session approaching SessionManager #8079

Closed

shinrich mentioned this pull request Sep 2, 2021

Fix crash during raw connect failures #8306

Merged

zwoop modified the milestones: 10.0.0, 9.2.0 Sep 23, 2021

cmcfarlen mentioned this pull request Aug 9, 2023

Segmentation fault with attach_server_session_to_client and Puma backend #10116

Closed


		sockaddr const *get_remote_addr() const;

		virtual HTTPVersion get_version(HTTPHdr &hdr) const;

Make HttpSM server reference a Transaction instead of a Session #7849

Make HttpSM server reference a Transaction instead of a Session #7849

Uh oh!

Conversation

shinrich commented May 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shinrich commented May 17, 2021

Uh oh!

shinrich commented May 17, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwoop May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maskit left a comment

Choose a reason for hiding this comment

Uh oh!

shinrich commented Jun 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

zwoop May 18, 2021 •

edited

Loading