diff --git a/ci/tsqa/tests/test_connect_attempts.py b/ci/tsqa/tests/test_connect_attempts.py index 58d70e1f5f3..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 @@ -111,6 +81,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): @@ -144,8 +144,8 @@ 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,)) + sock = _add_sock('slow_response') + t = threading.Thread(target=thread_slow_response, args=(sock,)) t.daemon = True t.start() @@ -200,12 +200,9 @@ 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']) + 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, 200) - # make sure its not the first one (otherwise the test messed up somehow) - print ret.text - self.assertGreater(int(ret.text), 0) + self.assertEqual(ret.status_code, 504) 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 085ebb5d1a3..89562f6a43c 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); @@ -6413,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;