From c3cc9716920ce8cc05f1a1c5683182b6c2208ca7 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 30 Nov 2017 14:16:42 -0800 Subject: [PATCH 1/7] properly shutdown endpoints in h2_http_proxy --- .../end2end/fixtures/http_proxy_fixture.cc | 90 ++++++++++++++----- 1 file changed, 70 insertions(+), 20 deletions(-) diff --git a/test/core/end2end/fixtures/http_proxy_fixture.cc b/test/core/end2end/fixtures/http_proxy_fixture.cc index ac0c953a79f..1cac0e98de5 100644 --- a/test/core/end2end/fixtures/http_proxy_fixture.cc +++ b/test/core/end2end/fixtures/http_proxy_fixture.cc @@ -68,6 +68,15 @@ struct grpc_end2end_http_proxy { // Connection handling // +#define SERVER_EP_READ_FAIL 1 +#define SERVER_EP_WRITE_FAIL 2 +#define CLIENT_EP_READ_FAIL 4 +#define CLIENT_EP_WRITE_FAIL 8 + +#define SERVER_EP_FAIL (SERVER_EP_READ_FAIL | SERVER_EP_WRITE_FAIL) +#define CLIENT_EP_FAIL (CLIENT_EP_READ_FAIL | CLIENT_EP_WRITE_FAIL) +#define EP_FAIL (SERVER_EP_FAIL | CLIENT_EP_FAIL) + typedef struct proxy_connection { grpc_end2end_http_proxy* proxy; @@ -76,6 +85,13 @@ typedef struct proxy_connection { gpr_refcount refcount; + // The lower four bits store the endpoint failure information + // bit-0 Server endpoint read failure + // bit-1 Server endpoint write failure + // bit-2 Client endpoint read failure + // bit-3 Client endpoint write failure + gpr_atm ep_state; + grpc_pollset_set* pollset_set; grpc_closure on_read_request_done; @@ -132,17 +148,50 @@ static void proxy_connection_unref(grpc_exec_ctx* exec_ctx, // Helper function to shut down the proxy connection. // Does NOT take ownership of a reference to error. static void proxy_connection_failed(grpc_exec_ctx* exec_ctx, - proxy_connection* conn, bool is_client, + proxy_connection* conn, int failure_type, const char* prefix, grpc_error* error) { const char* msg = grpc_error_string(error); gpr_log(GPR_INFO, "%s: %s", prefix, msg); - grpc_endpoint_shutdown(exec_ctx, conn->client_endpoint, - GRPC_ERROR_REF(error)); - if (conn->server_endpoint != nullptr) { - grpc_endpoint_shutdown(exec_ctx, conn->server_endpoint, - GRPC_ERROR_REF(error)); + gpr_atm ep_state; + gpr_atm new_ep_state; + while (true) { + ep_state = gpr_atm_no_barrier_load(&conn->ep_state); + new_ep_state = ep_state | failure_type; + + if (ep_state == new_ep_state) { + break; + } + + if (!gpr_atm_no_barrier_cas(&conn->ep_state, ep_state, new_ep_state)) { + continue; + } + + // failure_type is successfully set and new_ep_state != ep_state at this + // point + + // Shutdown the endpoint (client and/or server) if both read and write + // failures are observed after setting the failure_type. + // To prevent calling endpoint shutdown multiple times, It is important to + // ensure that ep_state i.e the old state, did not already have both + // failures set. + if (((ep_state & SERVER_EP_FAIL) != SERVER_EP_FAIL) && + ((new_ep_state & SERVER_EP_FAIL) == SERVER_EP_FAIL)) { + if (conn->server_endpoint != nullptr) { + grpc_endpoint_shutdown(exec_ctx, conn->server_endpoint, + GRPC_ERROR_REF(error)); + } + } + + if (((ep_state & CLIENT_EP_FAIL) != CLIENT_EP_FAIL) && + ((new_ep_state & CLIENT_EP_FAIL) == CLIENT_EP_FAIL)) { + grpc_endpoint_shutdown(exec_ctx, conn->client_endpoint, + GRPC_ERROR_REF(error)); + } + + break; } + proxy_connection_unref(exec_ctx, conn, "conn_failed"); } @@ -152,7 +201,7 @@ static void on_client_write_done(grpc_exec_ctx* exec_ctx, void* arg, proxy_connection* conn = (proxy_connection*)arg; conn->client_is_writing = false; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, true /* is_client */, + proxy_connection_failed(exec_ctx, conn, CLIENT_EP_WRITE_FAIL, "HTTP proxy client write", error); return; } @@ -179,7 +228,7 @@ static void on_server_write_done(grpc_exec_ctx* exec_ctx, void* arg, proxy_connection* conn = (proxy_connection*)arg; conn->server_is_writing = false; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, false /* is_client */, + proxy_connection_failed(exec_ctx, conn, SERVER_EP_WRITE_FAIL, "HTTP proxy server write", error); return; } @@ -206,7 +255,7 @@ static void on_client_read_done(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { proxy_connection* conn = (proxy_connection*)arg; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, true /* is_client */, + proxy_connection_failed(exec_ctx, conn, CLIENT_EP_READ_FAIL, "HTTP proxy client read", error); return; } @@ -239,7 +288,7 @@ static void on_server_read_done(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { proxy_connection* conn = (proxy_connection*)arg; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, false /* is_client */, + proxy_connection_failed(exec_ctx, conn, SERVER_EP_READ_FAIL, "HTTP proxy server read", error); return; } @@ -272,7 +321,7 @@ static void on_write_response_done(grpc_exec_ctx* exec_ctx, void* arg, proxy_connection* conn = (proxy_connection*)arg; conn->client_is_writing = false; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, true /* is_client */, + proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy write response", error); return; } @@ -301,7 +350,7 @@ static void on_server_connect_done(grpc_exec_ctx* exec_ctx, void* arg, // connection failed. However, for the purposes of this test code, // it's fine to pretend this is a client-side error, which will // cause the client connection to be dropped. - proxy_connection_failed(exec_ctx, conn, true /* is_client */, + proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy server connect", error); return; } @@ -351,8 +400,8 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, gpr_log(GPR_DEBUG, "on_read_request_done: %p %s", conn, grpc_error_string(error)); if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, true /* is_client */, - "HTTP proxy read request", error); + proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy read request", + error); return; } // Read request and feed it to the parser. @@ -361,7 +410,7 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, error = grpc_http_parser_parse( &conn->http_parser, conn->client_read_buffer.slices[i], nullptr); if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, true /* is_client */, + proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy request parse", error); GRPC_ERROR_UNREF(error); return; @@ -382,8 +431,8 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, conn->http_request.method); error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg); gpr_free(msg); - proxy_connection_failed(exec_ctx, conn, true /* is_client */, - "HTTP proxy read request", error); + proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy read request", + error); GRPC_ERROR_UNREF(error); return; } @@ -403,7 +452,7 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, if (!client_authenticated) { const char* msg = "HTTP Connect could not verify authentication"; error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(msg); - proxy_connection_failed(exec_ctx, conn, true /* is_client */, + proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy read request", error); GRPC_ERROR_UNREF(error); return; @@ -414,8 +463,8 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, error = grpc_blocking_resolve_address(conn->http_request.path, "80", &resolved_addresses); if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, true /* is_client */, - "HTTP proxy DNS lookup", error); + proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy DNS lookup", + error); GRPC_ERROR_UNREF(error); return; } @@ -441,6 +490,7 @@ static void on_accept(grpc_exec_ctx* exec_ctx, void* arg, conn->client_endpoint = endpoint; conn->proxy = proxy; gpr_ref_init(&conn->refcount, 1); + gpr_atm_no_barrier_store(&conn->ep_state, 0); conn->pollset_set = grpc_pollset_set_create(); grpc_pollset_set_add_pollset(exec_ctx, conn->pollset_set, proxy->pollset); grpc_endpoint_add_to_pollset_set(exec_ctx, endpoint, conn->pollset_set); From 02df4abc866d0205f4ab3bab5df1036a83d118e0 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 30 Nov 2017 14:51:44 -0800 Subject: [PATCH 2/7] Change ep_state from gpr_atm to int since all callbacks execute under a combiner and hence conn object is thread safe --- .../end2end/fixtures/http_proxy_fixture.cc | 26 +++++-------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/test/core/end2end/fixtures/http_proxy_fixture.cc b/test/core/end2end/fixtures/http_proxy_fixture.cc index 1cac0e98de5..c6a8abf40a9 100644 --- a/test/core/end2end/fixtures/http_proxy_fixture.cc +++ b/test/core/end2end/fixtures/http_proxy_fixture.cc @@ -90,7 +90,7 @@ typedef struct proxy_connection { // bit-1 Server endpoint write failure // bit-2 Client endpoint read failure // bit-3 Client endpoint write failure - gpr_atm ep_state; + int ep_state; grpc_pollset_set* pollset_set; @@ -153,22 +153,10 @@ static void proxy_connection_failed(grpc_exec_ctx* exec_ctx, const char* msg = grpc_error_string(error); gpr_log(GPR_INFO, "%s: %s", prefix, msg); - gpr_atm ep_state; - gpr_atm new_ep_state; - while (true) { - ep_state = gpr_atm_no_barrier_load(&conn->ep_state); - new_ep_state = ep_state | failure_type; - - if (ep_state == new_ep_state) { - break; - } - - if (!gpr_atm_no_barrier_cas(&conn->ep_state, ep_state, new_ep_state)) { - continue; - } - - // failure_type is successfully set and new_ep_state != ep_state at this - // point + int ep_state = conn->ep_state; + int new_ep_state = ep_state | failure_type; + if (ep_state != new_ep_state) { + conn->ep_state = new_ep_state; // Shutdown the endpoint (client and/or server) if both read and write // failures are observed after setting the failure_type. @@ -188,8 +176,6 @@ static void proxy_connection_failed(grpc_exec_ctx* exec_ctx, grpc_endpoint_shutdown(exec_ctx, conn->client_endpoint, GRPC_ERROR_REF(error)); } - - break; } proxy_connection_unref(exec_ctx, conn, "conn_failed"); @@ -490,7 +476,7 @@ static void on_accept(grpc_exec_ctx* exec_ctx, void* arg, conn->client_endpoint = endpoint; conn->proxy = proxy; gpr_ref_init(&conn->refcount, 1); - gpr_atm_no_barrier_store(&conn->ep_state, 0); + conn->ep_state = 0; conn->pollset_set = grpc_pollset_set_create(); grpc_pollset_set_add_pollset(exec_ctx, conn->pollset_set, proxy->pollset); grpc_endpoint_add_to_pollset_set(exec_ctx, endpoint, conn->pollset_set); From 445c5664e0572daa7b07fdde0e4959f0deeb803f Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 30 Nov 2017 15:02:15 -0800 Subject: [PATCH 3/7] Add special cases and some comments --- .../end2end/fixtures/http_proxy_fixture.cc | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/test/core/end2end/fixtures/http_proxy_fixture.cc b/test/core/end2end/fixtures/http_proxy_fixture.cc index c6a8abf40a9..f46edac6d76 100644 --- a/test/core/end2end/fixtures/http_proxy_fixture.cc +++ b/test/core/end2end/fixtures/http_proxy_fixture.cc @@ -77,6 +77,9 @@ struct grpc_end2end_http_proxy { #define CLIENT_EP_FAIL (CLIENT_EP_READ_FAIL | CLIENT_EP_WRITE_FAIL) #define EP_FAIL (SERVER_EP_FAIL | CLIENT_EP_FAIL) +// proxy_connection structure is only accessed in the closures which are all +// scheduled under the same combiner lock. So there is is no need for a mutex to +// protect this structure. typedef struct proxy_connection { grpc_end2end_http_proxy* proxy; @@ -94,6 +97,8 @@ typedef struct proxy_connection { grpc_pollset_set* pollset_set; + // NOTE: All the closures execute under proxy->combiner lock. Which means + // there will not be any data-races between the closures grpc_closure on_read_request_done; grpc_closure on_server_connect_done; grpc_closure on_write_response_done; @@ -241,8 +246,13 @@ static void on_client_read_done(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { proxy_connection* conn = (proxy_connection*)arg; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, CLIENT_EP_READ_FAIL, - "HTTP proxy client read", error); + // Report a read failure on the client endpoint. If there is no pending + // server write, then shutdown the server endpoint as well. + proxy_connection_failed( + exec_ctx, conn, + (conn->server_is_writing ? CLIENT_EP_READ_FAIL + : (CLIENT_EP_READ_FAIL | SERVER_EP_FAIL)), + "HTTP proxy client read", error); return; } // If there is already a pending write (i.e., server_write_buffer is @@ -274,8 +284,13 @@ static void on_server_read_done(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { proxy_connection* conn = (proxy_connection*)arg; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, SERVER_EP_READ_FAIL, - "HTTP proxy server read", error); + // Report a read failure on the server end point. If there is no pending + // write to the client, then shutdown the client endpoint as well. + proxy_connection_failed( + exec_ctx, conn, + (conn->client_is_writing ? SERVER_EP_READ_FAIL + : (SERVER_EP_READ_FAIL | CLIENT_EP_FAIL)), + "HTTP proxy server read", error); return; } // If there is already a pending write (i.e., client_write_buffer is From 906adfdb720bcbd9268079d664516b15b79ea485 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Fri, 1 Dec 2017 10:05:45 -0800 Subject: [PATCH 4/7] No need to explicitly zero initialize since we are doing zalloc. Some misc formatting changes --- test/core/end2end/fixtures/http_proxy_fixture.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/core/end2end/fixtures/http_proxy_fixture.cc b/test/core/end2end/fixtures/http_proxy_fixture.cc index f46edac6d76..31dfcd665e1 100644 --- a/test/core/end2end/fixtures/http_proxy_fixture.cc +++ b/test/core/end2end/fixtures/http_proxy_fixture.cc @@ -157,12 +157,10 @@ static void proxy_connection_failed(grpc_exec_ctx* exec_ctx, const char* prefix, grpc_error* error) { const char* msg = grpc_error_string(error); gpr_log(GPR_INFO, "%s: %s", prefix, msg); - int ep_state = conn->ep_state; int new_ep_state = ep_state | failure_type; if (ep_state != new_ep_state) { conn->ep_state = new_ep_state; - // Shutdown the endpoint (client and/or server) if both read and write // failures are observed after setting the failure_type. // To prevent calling endpoint shutdown multiple times, It is important to @@ -175,14 +173,12 @@ static void proxy_connection_failed(grpc_exec_ctx* exec_ctx, GRPC_ERROR_REF(error)); } } - if (((ep_state & CLIENT_EP_FAIL) != CLIENT_EP_FAIL) && ((new_ep_state & CLIENT_EP_FAIL) == CLIENT_EP_FAIL)) { grpc_endpoint_shutdown(exec_ctx, conn->client_endpoint, GRPC_ERROR_REF(error)); } } - proxy_connection_unref(exec_ctx, conn, "conn_failed"); } @@ -491,7 +487,6 @@ static void on_accept(grpc_exec_ctx* exec_ctx, void* arg, conn->client_endpoint = endpoint; conn->proxy = proxy; gpr_ref_init(&conn->refcount, 1); - conn->ep_state = 0; conn->pollset_set = grpc_pollset_set_create(); grpc_pollset_set_add_pollset(exec_ctx, conn->pollset_set, proxy->pollset); grpc_endpoint_add_to_pollset_set(exec_ctx, endpoint, conn->pollset_set); From 1af674aa2deab5b6e854f48aaa12aa36a9e7cd4e Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 1 Dec 2017 14:06:33 -0800 Subject: [PATCH 5/7] Provide a cleaner API for proxy_connection_failed(). --- .../end2end/fixtures/http_proxy_fixture.cc | 138 +++++++++--------- 1 file changed, 70 insertions(+), 68 deletions(-) diff --git a/test/core/end2end/fixtures/http_proxy_fixture.cc b/test/core/end2end/fixtures/http_proxy_fixture.cc index 31dfcd665e1..5c73ddb0033 100644 --- a/test/core/end2end/fixtures/http_proxy_fixture.cc +++ b/test/core/end2end/fixtures/http_proxy_fixture.cc @@ -68,15 +68,6 @@ struct grpc_end2end_http_proxy { // Connection handling // -#define SERVER_EP_READ_FAIL 1 -#define SERVER_EP_WRITE_FAIL 2 -#define CLIENT_EP_READ_FAIL 4 -#define CLIENT_EP_WRITE_FAIL 8 - -#define SERVER_EP_FAIL (SERVER_EP_READ_FAIL | SERVER_EP_WRITE_FAIL) -#define CLIENT_EP_FAIL (CLIENT_EP_READ_FAIL | CLIENT_EP_WRITE_FAIL) -#define EP_FAIL (SERVER_EP_FAIL | CLIENT_EP_FAIL) - // proxy_connection structure is only accessed in the closures which are all // scheduled under the same combiner lock. So there is is no need for a mutex to // protect this structure. @@ -88,13 +79,6 @@ typedef struct proxy_connection { gpr_refcount refcount; - // The lower four bits store the endpoint failure information - // bit-0 Server endpoint read failure - // bit-1 Server endpoint write failure - // bit-2 Client endpoint read failure - // bit-3 Client endpoint write failure - int ep_state; - grpc_pollset_set* pollset_set; // NOTE: All the closures execute under proxy->combiner lock. Which means @@ -107,6 +91,13 @@ typedef struct proxy_connection { grpc_closure on_server_read_done; grpc_closure on_server_write_done; + bool client_read_failed : 1; + bool client_write_failed : 1; + bool client_shutdown : 1; + bool server_read_failed : 1; + bool server_write_failed : 1; + bool server_shutdown : 1; + grpc_slice_buffer client_read_buffer; grpc_slice_buffer client_deferred_write_buffer; bool client_is_writing; @@ -150,36 +141,52 @@ static void proxy_connection_unref(grpc_exec_ctx* exec_ctx, } } +enum failure_type { + SETUP_FAILED, // To be used before we start proxying. + CLIENT_READ_FAILED, + CLIENT_WRITE_FAILED, + SERVER_READ_FAILED, + SERVER_WRITE_FAILED, +}; + // Helper function to shut down the proxy connection. -// Does NOT take ownership of a reference to error. static void proxy_connection_failed(grpc_exec_ctx* exec_ctx, - proxy_connection* conn, int failure_type, - const char* prefix, grpc_error* error) { - const char* msg = grpc_error_string(error); - gpr_log(GPR_INFO, "%s: %s", prefix, msg); - int ep_state = conn->ep_state; - int new_ep_state = ep_state | failure_type; - if (ep_state != new_ep_state) { - conn->ep_state = new_ep_state; - // Shutdown the endpoint (client and/or server) if both read and write - // failures are observed after setting the failure_type. - // To prevent calling endpoint shutdown multiple times, It is important to - // ensure that ep_state i.e the old state, did not already have both - // failures set. - if (((ep_state & SERVER_EP_FAIL) != SERVER_EP_FAIL) && - ((new_ep_state & SERVER_EP_FAIL) == SERVER_EP_FAIL)) { - if (conn->server_endpoint != nullptr) { - grpc_endpoint_shutdown(exec_ctx, conn->server_endpoint, - GRPC_ERROR_REF(error)); - } + proxy_connection* conn, + failure_type failure, const char* prefix, + grpc_error* error) { + gpr_log(GPR_INFO, "%s: %s", prefix, grpc_error_string(error)); + // Decide whether we should shut down the client and server. + bool shutdown_client = false; + bool shutdown_server = false; + if (failure == SETUP_FAILED) { + shutdown_client = true; + shutdown_server = true; + } else { + if ((failure == CLIENT_READ_FAILED && conn->client_write_failed) || + (failure == CLIENT_WRITE_FAILED && conn->client_read_failed) || + (failure == SERVER_READ_FAILED && !conn->client_is_writing)) { + shutdown_client = true; } - if (((ep_state & CLIENT_EP_FAIL) != CLIENT_EP_FAIL) && - ((new_ep_state & CLIENT_EP_FAIL) == CLIENT_EP_FAIL)) { - grpc_endpoint_shutdown(exec_ctx, conn->client_endpoint, - GRPC_ERROR_REF(error)); + if ((failure == SERVER_READ_FAILED && conn->server_write_failed) || + (failure == SERVER_WRITE_FAILED && conn->server_read_failed) || + (failure == CLIENT_READ_FAILED && !conn->server_is_writing)) { + shutdown_server = true; } } + // If we decided to shut down either one and have not yet done so, do so. + if (shutdown_client && !conn->client_shutdown) { + grpc_endpoint_shutdown(exec_ctx, conn->client_endpoint, + GRPC_ERROR_REF(error)); + conn->client_shutdown = true; + } + if (shutdown_server && !conn->server_shutdown) { + grpc_endpoint_shutdown(exec_ctx, conn->server_endpoint, + GRPC_ERROR_REF(error)); + conn->server_shutdown = true; + } + // Unref the connection. proxy_connection_unref(exec_ctx, conn, "conn_failed"); + GRPC_ERROR_UNREF(error); } // Callback for writing proxy data to the client. @@ -188,8 +195,8 @@ static void on_client_write_done(grpc_exec_ctx* exec_ctx, void* arg, proxy_connection* conn = (proxy_connection*)arg; conn->client_is_writing = false; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, CLIENT_EP_WRITE_FAIL, - "HTTP proxy client write", error); + proxy_connection_failed(exec_ctx, conn, CLIENT_WRITE_FAILED, + "HTTP proxy client write", GRPC_ERROR_REF(error)); return; } // Clear write buffer (the data we just wrote). @@ -215,8 +222,8 @@ static void on_server_write_done(grpc_exec_ctx* exec_ctx, void* arg, proxy_connection* conn = (proxy_connection*)arg; conn->server_is_writing = false; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, SERVER_EP_WRITE_FAIL, - "HTTP proxy server write", error); + proxy_connection_failed(exec_ctx, conn, SERVER_WRITE_FAILED, + "HTTP proxy server write", GRPC_ERROR_REF(error)); return; } // Clear write buffer (the data we just wrote). @@ -244,11 +251,8 @@ static void on_client_read_done(grpc_exec_ctx* exec_ctx, void* arg, if (error != GRPC_ERROR_NONE) { // Report a read failure on the client endpoint. If there is no pending // server write, then shutdown the server endpoint as well. - proxy_connection_failed( - exec_ctx, conn, - (conn->server_is_writing ? CLIENT_EP_READ_FAIL - : (CLIENT_EP_READ_FAIL | SERVER_EP_FAIL)), - "HTTP proxy client read", error); + proxy_connection_failed(exec_ctx, conn, CLIENT_READ_FAILED, + "HTTP proxy client read", GRPC_ERROR_REF(error)); return; } // If there is already a pending write (i.e., server_write_buffer is @@ -282,11 +286,8 @@ static void on_server_read_done(grpc_exec_ctx* exec_ctx, void* arg, if (error != GRPC_ERROR_NONE) { // Report a read failure on the server end point. If there is no pending // write to the client, then shutdown the client endpoint as well. - proxy_connection_failed( - exec_ctx, conn, - (conn->client_is_writing ? SERVER_EP_READ_FAIL - : (SERVER_EP_READ_FAIL | CLIENT_EP_FAIL)), - "HTTP proxy server read", error); + proxy_connection_failed(exec_ctx, conn, SERVER_READ_FAILED, + "HTTP proxy server read", GRPC_ERROR_REF(error)); return; } // If there is already a pending write (i.e., client_write_buffer is @@ -318,8 +319,8 @@ static void on_write_response_done(grpc_exec_ctx* exec_ctx, void* arg, proxy_connection* conn = (proxy_connection*)arg; conn->client_is_writing = false; if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, EP_FAIL, - "HTTP proxy write response", error); + proxy_connection_failed(exec_ctx, conn, SETUP_FAILED, + "HTTP proxy write response", GRPC_ERROR_REF(error)); return; } // Clear write buffer. @@ -347,8 +348,8 @@ static void on_server_connect_done(grpc_exec_ctx* exec_ctx, void* arg, // connection failed. However, for the purposes of this test code, // it's fine to pretend this is a client-side error, which will // cause the client connection to be dropped. - proxy_connection_failed(exec_ctx, conn, EP_FAIL, - "HTTP proxy server connect", error); + proxy_connection_failed(exec_ctx, conn, SETUP_FAILED, + "HTTP proxy server connect", GRPC_ERROR_REF(error)); return; } // We've established a connection, so send back a 200 response code to @@ -397,8 +398,8 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, gpr_log(GPR_DEBUG, "on_read_request_done: %p %s", conn, grpc_error_string(error)); if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy read request", - error); + proxy_connection_failed(exec_ctx, conn, SETUP_FAILED, + "HTTP proxy read request", GRPC_ERROR_REF(error)); return; } // Read request and feed it to the parser. @@ -407,8 +408,9 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, error = grpc_http_parser_parse( &conn->http_parser, conn->client_read_buffer.slices[i], nullptr); if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, EP_FAIL, - "HTTP proxy request parse", error); + proxy_connection_failed(exec_ctx, conn, SETUP_FAILED, + "HTTP proxy request parse", + GRPC_ERROR_REF(error)); GRPC_ERROR_UNREF(error); return; } @@ -428,8 +430,8 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, conn->http_request.method); error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg); gpr_free(msg); - proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy read request", - error); + proxy_connection_failed(exec_ctx, conn, SETUP_FAILED, + "HTTP proxy read request", GRPC_ERROR_REF(error)); GRPC_ERROR_UNREF(error); return; } @@ -449,8 +451,8 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, if (!client_authenticated) { const char* msg = "HTTP Connect could not verify authentication"; error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(msg); - proxy_connection_failed(exec_ctx, conn, EP_FAIL, - "HTTP proxy read request", error); + proxy_connection_failed(exec_ctx, conn, SETUP_FAILED, + "HTTP proxy read request", GRPC_ERROR_REF(error)); GRPC_ERROR_UNREF(error); return; } @@ -460,8 +462,8 @@ static void on_read_request_done(grpc_exec_ctx* exec_ctx, void* arg, error = grpc_blocking_resolve_address(conn->http_request.path, "80", &resolved_addresses); if (error != GRPC_ERROR_NONE) { - proxy_connection_failed(exec_ctx, conn, EP_FAIL, "HTTP proxy DNS lookup", - error); + proxy_connection_failed(exec_ctx, conn, SETUP_FAILED, + "HTTP proxy DNS lookup", GRPC_ERROR_REF(error)); GRPC_ERROR_UNREF(error); return; } From e37104cf428bf4438ea8cce6061df8807bfec364 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Fri, 1 Dec 2017 14:26:08 -0800 Subject: [PATCH 6/7] Delete outdated comments --- test/core/end2end/fixtures/http_proxy_fixture.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/core/end2end/fixtures/http_proxy_fixture.cc b/test/core/end2end/fixtures/http_proxy_fixture.cc index 5c73ddb0033..190ea78ef9a 100644 --- a/test/core/end2end/fixtures/http_proxy_fixture.cc +++ b/test/core/end2end/fixtures/http_proxy_fixture.cc @@ -249,8 +249,6 @@ static void on_client_read_done(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { proxy_connection* conn = (proxy_connection*)arg; if (error != GRPC_ERROR_NONE) { - // Report a read failure on the client endpoint. If there is no pending - // server write, then shutdown the server endpoint as well. proxy_connection_failed(exec_ctx, conn, CLIENT_READ_FAILED, "HTTP proxy client read", GRPC_ERROR_REF(error)); return; @@ -284,8 +282,6 @@ static void on_server_read_done(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { proxy_connection* conn = (proxy_connection*)arg; if (error != GRPC_ERROR_NONE) { - // Report a read failure on the server end point. If there is no pending - // write to the client, then shutdown the client endpoint as well. proxy_connection_failed(exec_ctx, conn, SERVER_READ_FAILED, "HTTP proxy server read", GRPC_ERROR_REF(error)); return; From 4ca35636fe3c5d1e936d4cc03d18efb4be2824b8 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Mon, 4 Dec 2017 12:13:50 -0800 Subject: [PATCH 7/7] fix connectivity test failure --- test/core/end2end/fixtures/http_proxy_fixture.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/core/end2end/fixtures/http_proxy_fixture.cc b/test/core/end2end/fixtures/http_proxy_fixture.cc index 190ea78ef9a..0cec27bad7d 100644 --- a/test/core/end2end/fixtures/http_proxy_fixture.cc +++ b/test/core/end2end/fixtures/http_proxy_fixture.cc @@ -179,7 +179,8 @@ static void proxy_connection_failed(grpc_exec_ctx* exec_ctx, GRPC_ERROR_REF(error)); conn->client_shutdown = true; } - if (shutdown_server && !conn->server_shutdown) { + if (shutdown_server && !conn->server_shutdown && + (conn->server_endpoint != nullptr)) { grpc_endpoint_shutdown(exec_ctx, conn->server_endpoint, GRPC_ERROR_REF(error)); conn->server_shutdown = true;