[trafficserver] branch master updated: TS-4509 Add `outstanding_bytes` to VConnection

[ja...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jacksontj pushed a commit to branch master
in repository https://git-dual.apache.org/repos/asf/trafficserver.git

The following commit(s) were added to refs/heads/master by this push:
       new  3c7d1b1   TS-4509 Add `outstanding_bytes` to VConnection
3c7d1b1 is described below

commit 3c7d1b1f341c3cd28275774eeb994c3d4883277e
Author: Thomas Jackson <ja...@gmail.com>
AuthorDate: Mon Oct 3 08:16:28 2016 -0700

    TS-4509 Add `outstanding_bytes` to VConnection
    
    With this we can better check request retryability. This (in addition to not releasing the sessions immediately on error) means that if the request is retryable we can simply check if the number of bytes queued is the same as the number of bytes we've asked to write. If these match, then we can be sure we didn't send any ACKd packets-- meaning we are completely safe to retry.
---
 iocore/net/I_NetVConnection.h     |  8 ++++++++
 iocore/net/P_UnixNetVConnection.h |  1 +
 iocore/net/UnixNetVConnection.cc  | 13 +++++++++++++
 proxy/http/HttpSM.cc              |  6 ------
 proxy/http/HttpTransact.cc        | 17 +++++++++++++----
 5 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/iocore/net/I_NetVConnection.h b/iocore/net/I_NetVConnection.h
index 3216944..c02ceb5 100644
--- a/iocore/net/I_NetVConnection.h
+++ b/iocore/net/I_NetVConnection.h
@@ -242,6 +242,14 @@ private:
 class NetVConnection : public VConnection
 {
 public:
+  // How many bytes have been queued to the OS for sending by haven't been sent yet
+  // Not all platforms support this, and if they don't we'll return -1 for them
+  virtual const int64_t
+  outstanding()
+  {
+    return -1;
+  };
+
   /**
      Initiates read. Thread safe, may be called when not handling
      an event from the NetVConnection, or the NetVConnection creation
diff --git a/iocore/net/P_UnixNetVConnection.h b/iocore/net/P_UnixNetVConnection.h
index 8725efd..b28202b 100644
--- a/iocore/net/P_UnixNetVConnection.h
+++ b/iocore/net/P_UnixNetVConnection.h
@@ -100,6 +100,7 @@ struct OOB_callback : public Continuation {
 class UnixNetVConnection : public NetVConnection
 {
 public:
+  virtual const int64_t outstanding();
   virtual VIO *do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf);
   virtual VIO *do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *buf, bool owner = false);
 
diff --git a/iocore/net/UnixNetVConnection.cc b/iocore/net/UnixNetVConnection.cc
index 7c18e27..a4376e9 100644
--- a/iocore/net/UnixNetVConnection.cc
+++ b/iocore/net/UnixNetVConnection.cc
@@ -632,6 +632,19 @@ UnixNetVConnection::get_data(int id, void *data)
   }
 }
 
+const int64_t
+UnixNetVConnection::outstanding()
+{
+  int n;
+  int ret = ioctl(this->get_socket(), TIOCOUTQ, &n);
+  // if there was an error (such as ioctl doesn't support this call on this platform) then
+  // we return -1
+  if (ret == -1) {
+    return ret;
+  }
+  return n;
+}
+
 VIO *
 UnixNetVConnection::do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf)
 {
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index 4cd28b1..11cfa33 100644
--- a/proxy/http/HttpSM.cc
+++ b/proxy/http/HttpSM.cc
@@ -5313,9 +5313,6 @@ HttpSM::handle_post_failure()
     tunnel.deallocate_buffers();
     tunnel.reset();
     // Server died
-    vc_table.cleanup_entry(server_entry);
-    server_entry          = NULL;
-    server_session        = NULL;
     t_state.current.state = HttpTransact::CONNECTION_CLOSED;
     call_transact_and_set_next_state(HttpTransact::HandleResponse);
   }
@@ -5477,9 +5474,6 @@ HttpSM::handle_server_setup_error(int event, void *data)
 
   // Closedown server connection and deallocate buffers
   ink_assert(server_entry->in_tunnel == false);
-  vc_table.cleanup_entry(server_entry);
-  server_entry   = NULL;
-  server_session = NULL;
 
   // if we are waiting on a plugin callout for
   //   HTTP_API_SEND_REQUEST_HDR defer calling transact until
diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc
index 87888d3..6382edb 100644
--- a/proxy/http/HttpTransact.cc
+++ b/proxy/http/HttpTransact.cc
@@ -6533,16 +6533,25 @@ HttpTransact::is_request_valid(State *s, HTTPHdr *incoming_request)
 // bool HttpTransact::is_request_retryable
 //
 // In the general case once bytes have been sent on the wire the request cannot be retried.
-// The reason we cannot retry is that the rfc2616 does not make any gaurantees about the
+// The reason we cannot retry is that the rfc2616 does not make any guarantees about the
 // retry-ability of a request. In fact in the reverse proxy case it is quite common for GET
-// requests on the origin to fire tracking events etc. So, as a proxy once we have sent bytes
-// on the wire to the server we cannot gaurantee that the request is safe to redispatch to another server.
+// requests on the origin to fire tracking events etc. So, as a proxy, once bytes have been ACKd
+// by the server we cannot guarantee that the request is safe to retry or redispatch to another server.
+// This is distinction of "ACKd" vs "sent" is intended, and has reason. In the case of a
+// new origin connection there is little difference, as the chance of a RST between setup
+// and the first set of bytes is relatively small. This distinction is more apparent in the
+// case where the origin connection is a KA session. In this case, the session may not have
+// been used for a long time. In that case, we'll immediately queue up session to send to the
+// origin, without any idea of the state of the connection. If the origin is dead (or the connection
+// is broken for some other reason) we'll immediately get a RST back. In that case-- since no
+// bytes where ACKd by the remote end, we can retry/redispatch the request.
 //
 bool
 HttpTransact::is_request_retryable(State *s)
 {
   // If there was no error establishing the connection (and we sent bytes)-- we cannot retry
-  if (s->current.state != CONNECTION_ERROR && s->state_machine->server_request_hdr_bytes > 0) {
+  if (s->current.state != CONNECTION_ERROR && s->state_machine->server_request_hdr_bytes > 0 &&
+      s->state_machine->get_server_session()->get_netvc()->outstanding() != s->state_machine->server_request_hdr_bytes) {
     return false;
   }
 

-- 
To stop receiving notification emails like this one, please contact
['"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>'].