TS-4509: Add `outstanding_bytes` to VConnection #1070

jacksontj · 2016-10-03T15:17:20Z

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.

atsci · 2016-10-03T15:33:25Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/915/ for details.

atsci · 2016-10-03T15:34:47Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/810/ for details.

atsci · 2016-10-03T15:51:11Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/916/ for details.

atsci · 2016-10-03T15:54:42Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/811/ for details.

jpeach · 2016-10-03T15:46:07Z

iocore/net/I_NetVConnection.h

+  // 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 int
+  oustanding_bytes()


s/oustanding_bytes/outstanding/

jpeach · 2016-10-03T15:47:02Z

iocore/net/I_NetVConnection.h

+  // 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 int
+  oustanding_bytes()


int64_t outstanding_bytes() const

Since we use int64_t for byte counts.

jpeach · 2016-10-03T15:47:42Z

iocore/net/UnixNetVConnection.cc

+UnixNetVConnection::oustanding_bytes()
+{
+  int n;
+  ioctl(this->get_socket(), TIOCOUTQ, &n);


Check the return, non-Linux doesn't support TIOCOUTQ usage in this way.

jpeach · 2016-10-03T15:47:55Z

iocore/net/UnixNetVConnection.cc

+  int n;
+  ioctl(this->get_socket(), TIOCOUTQ, &n);
+  return n;
+};


No trailing ;.

jpeach · 2016-10-03T15:48:19Z

proxy/http/HttpTransact.cc

-// 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
+// to the server we cannot gaurantee that the request is safe to retry or redispatch to another server.
+// This is destinction of "ACKd" vs "sent" is intended, and has reason. In the case of a


s/destinction/distinction/

jpeach · 2016-10-03T15:48:40Z

proxy/http/HttpTransact.cc

@@ -6536,14 +6536,23 @@ HttpTransact::is_request_valid(State *s, HTTPHdr *incoming_request)
 // 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
 // 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


So, as a proxy, once ...

jpeach · 2016-10-03T15:48:50Z

proxy/http/HttpTransact.cc

-// 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
+// to the server we cannot gaurantee that the request is safe to retry or redispatch to another server.


by the server

jpeach · 2016-10-03T15:49:06Z

proxy/http/HttpTransact.cc

-// 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
+// to the server we cannot gaurantee that the request is safe to retry or redispatch to another server.


s/gaurantee/guarantee/

jpeach · 2016-10-03T15:49:31Z

proxy/http/HttpTransact.cc

+// to the server we cannot gaurantee that the request is safe to retry or redispatch to another server.
+// This is destinction 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 destinction is more apparent in the


s/destinction/distinction/

jpeach · 2016-10-03T15:49:46Z

proxy/http/HttpTransact.cc

+// This is destinction 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 destinction is more apparent in the
+// case where the origin connection is a KA session. In this case, the seesion may not have


s/seession/session/

atsci · 2016-10-03T16:26:12Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/917/ for details.

atsci · 2016-10-03T16:29:41Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/812/ for details.

atsci · 2016-10-03T17:04:41Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/813/ for details.

atsci · 2016-10-03T17:08:45Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/919/ for details.

atsci · 2016-10-03T18:11:17Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/920/ for details.

atsci · 2016-10-03T18:14:44Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/814/ for details.

jacksontj · 2016-10-03T18:56:48Z

@jpeach I've updated the PR with your feedback-- although I'm not sure how to mark it as "done"-- its still showing that you have changes requested which is odd :/

atsci · 2016-10-03T21:54:47Z

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/816/ for details.

atsci · 2016-10-03T22:01:19Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/922/ for details.

jpeach

Please use the "TS-xxxx: description" convention for the PR and commit subjects.

jpeach · 2016-10-03T22:28:35Z

proxy/http/HttpSM.cc

@@ -5310,9 +5310,6 @@ HttpSM::handle_post_failure()
    tunnel.deallocate_buffers();
    tunnel.reset();
    // Server died
-    vc_table.cleanup_entry(server_entry);
-    server_entry          = NULL;
-    server_session        = NULL;


But at this point, we don't want to leave dangling pointers even if we do want to retry, right? If we retry we ought to get a new server session.

We should get a new session-- but to do the "is retryable" check, I need access to the FD-- which is problematic if we release the session-- since it get cleared/closed etc. With these lines removed it seems that we aren't leaking sessions (from testing locally and in our staging env).

jpeach · 2016-10-03T22:29:09Z

iocore/net/I_NetVConnection.h

+  // 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 int64_t
+  outstanding()


outstanding() const

atsci · 2016-10-04T16:38:16Z

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/925/ for details.

atsci · 2016-10-04T16:44:44Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/819/ for details.

atsci · 2016-10-04T16:48:13Z

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/926/ for details.

atsci · 2016-10-04T16:59:42Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/820/ for details.

atsci · 2016-10-04T17:08:28Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/927/ for details.

atsci · 2016-10-04T17:10:06Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/821/ for details.

jpeach · 2016-10-08T04:32:43Z

+1, assuming you are going to squash :)

jacksontj · 2016-10-10T20:29:36Z

Definitely going to squash :)

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.

atsci · 2016-10-11T01:16:13Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/956/ for details.

atsci · 2016-10-11T01:19:42Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/848/ for details.

jacksontj force-pushed the TS-3959 branch from 7605659 to 83acdc9 Compare October 3, 2016 15:39

jpeach requested changes Oct 3, 2016

View reviewed changes

jacksontj force-pushed the TS-3959 branch from 83acdc9 to 88fbde2 Compare October 3, 2016 16:14

jacksontj force-pushed the TS-3959 branch from 9c105d2 to cb34ff2 Compare October 3, 2016 16:51

jacksontj force-pushed the TS-3959 branch from cb34ff2 to 60385d5 Compare October 3, 2016 17:58

jpeach requested changes Oct 3, 2016

View reviewed changes

jacksontj changed the title ~~TS-4509 Add outstanding_bytes to VConnection~~ TS-4509: Add outstanding_bytes to VConnection Oct 4, 2016

jacksontj force-pushed the TS-3959 branch from 75ae060 to 9b7f0f6 Compare October 4, 2016 16:54

zwoop added the Network label Oct 4, 2016

zwoop added this to the 7.1.0 milestone Oct 4, 2016

zwoop assigned jpeach Oct 4, 2016

zwoop assigned SolidWallOfCode Oct 4, 2016

jpeach approved these changes Oct 8, 2016

View reviewed changes

jacksontj force-pushed the TS-3959 branch from 9b7f0f6 to b77431a Compare October 11, 2016 01:04

jacksontj merged commit 3c7d1b1 into apache:master Oct 11, 2016

hankai17 mentioned this pull request May 13, 2019

In HttpSM::handle_post_failure, We should clear vc imm? #5483

Closed

TS-4509: Add outstanding_bytes to VConnection #1070

TS-4509: Add outstanding_bytes to VConnection #1070

Conversation

jacksontj commented Oct 3, 2016

atsci commented Oct 3, 2016

atsci commented Oct 3, 2016

atsci commented Oct 3, 2016

atsci commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsci commented Oct 3, 2016

atsci commented Oct 3, 2016

atsci commented Oct 3, 2016

atsci commented Oct 3, 2016

atsci commented Oct 3, 2016

atsci commented Oct 3, 2016

jacksontj commented Oct 3, 2016

atsci commented Oct 3, 2016

atsci commented Oct 3, 2016

jpeach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsci commented Oct 4, 2016

atsci commented Oct 4, 2016

atsci commented Oct 4, 2016

atsci commented Oct 4, 2016

atsci commented Oct 4, 2016

atsci commented Oct 4, 2016

jpeach commented Oct 8, 2016

jacksontj commented Oct 10, 2016

atsci commented Oct 11, 2016

atsci commented Oct 11, 2016

TS-4509: Add `outstanding_bytes` to VConnection #1070

TS-4509: Add `outstanding_bytes` to VConnection #1070