From 59c3ed195acc2cac719eade3981411ab97cb4b6e Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Wed, 6 Apr 2016 18:58:12 -0700 Subject: [PATCH 1/4] Revert "TS-3440: Connect retries shouldn't happen if the connection has succeeeded and data has been sent" This reverts commit 440a14513e59baf21e55b07f0dd4aa39bac232de. Conflicts: proxy/http/HttpTransact.cc --- proxy/http/HttpTransact.cc | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index 085ebb5d1a3..c861ae4b154 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -3657,19 +3657,6 @@ HttpTransact::handle_response_from_server(State *s) s->current.server->set_connect_fail(EUSERS); // too many users handle_server_connection_not_open(s); break; - case CONNECTION_CLOSED: - /* fall through */ - case PARSE_ERROR: - /* fall through */ - case BAD_INCOMING_RESPONSE: { - // this case should not be allowed to retry because we'll end up making another request - DebugTxn("http_trans", - "[handle_response_from_server] Transaction received a bad response or a partial response, not retrying..."); - SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_FAILURE); - handle_server_died(s); - s->next_action = SM_ACTION_SEND_ERROR_CACHE_NOOP; - break; - } case OPEN_RAW_ERROR: /* fall through */ case CONNECTION_ERROR: @@ -3677,6 +3664,12 @@ HttpTransact::handle_response_from_server(State *s) case STATE_UNDEFINED: /* fall through */ case INACTIVE_TIMEOUT: + /* fall through */ + case PARSE_ERROR: + /* fall through */ + case CONNECTION_CLOSED: + /* fall through */ + case BAD_INCOMING_RESPONSE: // Set to generic I/O error if not already set specifically. if (!s->current.server->had_connect_fail()) s->current.server->set_connect_fail(EIO); From f129996e1adaab399359cd248f4750a686d6bfc5 Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Wed, 6 Apr 2016 18:58:36 -0700 Subject: [PATCH 2/4] TS-4328 Not retry if any bytes were sent Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the SM to see if we wrote any header bytes to the wire. If any bytes were sent the request cannot be retried. This closes #554 This also deprecates the `request_body_start` field, and as such removes it --- proxy/http/HttpSM.cc | 1 - proxy/http/HttpTransact.cc | 10 +++++++--- proxy/http/HttpTransact.h | 3 +-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index 5d58b757817..12cd4e622ce 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -5418,7 +5418,6 @@ HttpSM::do_setup_post_tunnel(HttpVC_t to_vc_type) IOBufferReader *buf_start = post_buffer->alloc_reader(); int64_t post_bytes = chunked ? INT64_MAX : t_state.hdr_info.request_content_length; - t_state.hdr_info.request_body_start = true; // Note: Many browsers, Netscape and IE included send two extra // bytes (CRLF) at the end of the post. We just ignore those // bytes since the sending them is not spec diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index c861ae4b154..89562f6a43c 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -6406,13 +6406,17 @@ HttpTransact::is_request_valid(State *s, HTTPHdr *incoming_request) // bool HttpTransact::is_request_retryable // -// If we started a POST/PUT tunnel then we can -// not retry failed requests +// 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. // bool HttpTransact::is_request_retryable(State *s) { - if (s->hdr_info.request_body_start == true) { + // If the connection was established-- we cannot retry + if (s->state_machine->server_request_hdr_bytes > 0) { return false; } diff --git a/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h index 7a1891f57b0..7b889c64047 100644 --- a/proxy/http/HttpTransact.h +++ b/proxy/http/HttpTransact.h @@ -745,13 +745,12 @@ class HttpTransact bool trust_response_cl; ResponseError_t response_error; bool extension_method; - bool request_body_start; _HeaderInfo() : client_request(), client_response(), server_request(), server_response(), transform_response(), cache_response(), request_content_length(HTTP_UNDEFINED_CL), response_content_length(HTTP_UNDEFINED_CL), transform_request_cl(HTTP_UNDEFINED_CL), transform_response_cl(HTTP_UNDEFINED_CL), client_req_is_server_style(false), - trust_response_cl(false), response_error(NO_RESPONSE_HEADER_ERROR), extension_method(false), request_body_start(false) + trust_response_cl(false), response_error(NO_RESPONSE_HEADER_ERROR), extension_method(false) { } } HeaderInfo; From a6e89ed4ea1a67613c27a4136e3a7a58ef7aead7 Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Wed, 6 Apr 2016 19:50:19 -0700 Subject: [PATCH 3/4] TS-4328: Add a test for the slow response case If the request hits a timeout-- but we sent the request to the origin we cannot rety the request --- ci/tsqa/tests/test_connect_attempts.py | 42 ++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/ci/tsqa/tests/test_connect_attempts.py b/ci/tsqa/tests/test_connect_attempts.py index 58d70e1f5f3..fc037aea625 100644 --- a/ci/tsqa/tests/test_connect_attempts.py +++ b/ci/tsqa/tests/test_connect_attempts.py @@ -111,6 +111,36 @@ def thread_partial_response(sock): connection.close() +def thread_slow_response(sock): + ''' + Thread to sleep a decreasing amount of time before sending the response + + sleep times: 2 -> 1 -> 0 + ''' + sock.listen(0) + sleep_time = 2 + num_requests = 0 + # poll + while True: + select.select([sock], [], []) + try: + connection, addr = sock.accept() + time.sleep(sleep_time) + connection.send(( + 'HTTP/1.1 200 OK\r\n' + 'Content-Length: {body_len}\r\n' + 'Content-Type: text/html; charset=UTF-8\r\n' + 'Connection: close\r\n\r\n{body}'.format(body_len=len(str(num_requests)), body=num_requests) + )) + connection.close() + num_requests += 1 + except Exception as e: + print 'connection died!', e + pass + if sleep_time > 0: + sleep_time -= 1 + + class TestOriginServerConnectAttempts(helpers.EnvironmentCase): @classmethod def setUpEnv(cls, env): @@ -149,6 +179,11 @@ def _add_sock(name): t.daemon = True t.start() + sock = _add_sock('slow_response') + t = threading.Thread(target=thread_slow_response, args=(sock,)) + t.daemon = True + t.start() + sock = _add_sock('partial_response') t = threading.Thread(target=thread_partial_response, args=(sock,)) t.daemon = True @@ -209,3 +244,10 @@ def test_delayed_accept_after_connect_origin(self): # make sure its not the first one (otherwise the test messed up somehow) print ret.text self.assertGreater(int(ret.text), 0) + + def test_slow_response(self): + '''Verify that we get 5xx from origins that take longer than acceptable, since we will not retry them''' + url = 'http://127.0.0.1:{0}/slow_response/s'.format(self.configs['records.config']['CONFIG']['proxy.config.http.server_ports']) + ret = requests.get(url) + # make sure it worked + self.assertEqual(ret.status_code, 504) From 770ca6fb2f10ea91d66a91ebca7e7689be174cb8 Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Wed, 6 Apr 2016 19:51:11 -0700 Subject: [PATCH 4/4] TS-4328: removing flawed test case This test case was intended to test the case where the TCP session was established but no bytes were sent. The issue is that user space has no control over the SYN-ACK, accept() simply is user-space saying "i want this connection". So because of that this test is useless, and can be removed --- ci/tsqa/tests/test_connect_attempts.py | 45 -------------------------- 1 file changed, 45 deletions(-) diff --git a/ci/tsqa/tests/test_connect_attempts.py b/ci/tsqa/tests/test_connect_attempts.py index fc037aea625..231f94d368e 100644 --- a/ci/tsqa/tests/test_connect_attempts.py +++ b/ci/tsqa/tests/test_connect_attempts.py @@ -38,36 +38,6 @@ def thread_die_on_connect(sock): sock.close() -def thread_delayed_accept_after_connect(sock): - ''' - Thread to sleep a decreasing amount of time before requests - - sleep times: 2 -> 1 -> 0 - ''' - sock.listen(0) - sleep_time = 2 - num_requests = 0 - # poll - while True: - select.select([sock], [], []) - time.sleep(sleep_time) - try: - connection, addr = sock.accept() - connection.send(( - 'HTTP/1.1 200 OK\r\n' - 'Content-Length: {body_len}\r\n' - 'Content-Type: text/html; charset=UTF-8\r\n' - 'Connection: close\r\n\r\n{body}'.format(body_len=len(str(num_requests)), body=num_requests) - )) - connection.close() - num_requests += 1 - except Exception as e: - print 'connection died!', e - pass - if sleep_time > 0: - sleep_time -= 1 - - def thread_reset_after_accept(sock): sock.listen(0) first = True @@ -174,11 +144,6 @@ def _add_sock(name): t.daemon = True t.start() - sock = _add_sock('delayed_accept_after_connect') - t = threading.Thread(target=thread_delayed_accept_after_connect, args=(sock,)) - t.daemon = True - t.start() - sock = _add_sock('slow_response') t = threading.Thread(target=thread_slow_response, args=(sock,)) t.daemon = True @@ -235,16 +200,6 @@ def test_reset_after_accept_origin(self): ret = requests.get(url) self.assertEqual(ret.status_code, 502) - def test_delayed_accept_after_connect_origin(self): - '''Verify that we get 200s from origins that delayed_accept_after_connect''' - url = 'http://127.0.0.1:{0}/delayed_accept_after_connect/s'.format(self.configs['records.config']['CONFIG']['proxy.config.http.server_ports']) - ret = requests.get(url) - # make sure it worked - self.assertEqual(ret.status_code, 200) - # make sure its not the first one (otherwise the test messed up somehow) - print ret.text - self.assertGreater(int(ret.text), 0) - def test_slow_response(self): '''Verify that we get 5xx from origins that take longer than acceptable, since we will not retry them''' url = 'http://127.0.0.1:{0}/slow_response/s'.format(self.configs['records.config']['CONFIG']['proxy.config.http.server_ports'])