From 8adcf3d0bd0a940b05a8c89d04b5327a478da1b5 Mon Sep 17 00:00:00 2001 From: Stephan Bosch Date: Mon, 21 Nov 2016 23:19:26 +0100 Subject: [PATCH] lib-http: client: Fixed assert failure occurring when a new connection fails for a peer that has active connections. Fixes: Panic: file http-client-queue.c: line 481 (http_client_queue_connection_failure): assertion failed: (queue->cur_peer == NULL) --- src/lib-http/http-client-queue.c | 13 ++- src/lib-http/test-http-client-errors.c | 133 +++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 2 deletions(-) diff --git a/src/lib-http/http-client-queue.c b/src/lib-http/http-client-queue.c index 63c2185237..e2e91897d1 100644 --- a/src/lib-http/http-client-queue.c +++ b/src/lib-http/http-client-queue.c @@ -478,8 +478,17 @@ http_client_queue_connection_failure(struct http_client_queue *queue, struct http_client_peer *failed_peer; struct http_client_peer *const *peer_idx; - i_assert(queue->cur_peer == NULL); - i_assert(array_count(&queue->pending_peers) > 0); + if (queue->cur_peer != NULL) { + /* The peer still has some working connections, which means that + pending requests wait until they're picked up by those connections + or the remaining connections fail as well. In the latter case, + connecting to different peer can resolve the situation, but only + if there is more than one IP. In any other case, the requests will + eventually fail. In the future we could start connections to the next + IP at this point already, but that is no small change. */ + i_assert(array_count(&queue->pending_peers) == 0); + return; + } http_client_queue_debug(queue, "Failed to set up connection to %s%s: %s " diff --git a/src/lib-http/test-http-client-errors.c b/src/lib-http/test-http-client-errors.c index 8eb1adf050..3df2da15ec 100644 --- a/src/lib-http/test-http-client-errors.c +++ b/src/lib-http/test-http-client-errors.c @@ -2192,6 +2192,138 @@ static void test_dns_lookup_ttl(void) test_end(); } +/* + * Peer reuse failure + */ + +/* server */ + +static void +test_peer_reuse_failure_input(struct server_connection *conn) +{ + static unsigned int seq = 0; + static const char *resp = + "HTTP/1.1 200 OK\r\n" + "\r\n"; + o_stream_nsend_str(conn->conn.output, resp); + if (seq++ > 2) { + server_connection_deinit(&conn); + io_loop_stop(current_ioloop); + } +} + +static void test_server_peer_reuse_failure(unsigned int index) +{ + test_server_input = test_peer_reuse_failure_input; + test_server_run(index); +} + +/* client */ + +struct _peer_reuse_failure { + struct timeout *to; + bool first:1; +}; + +static void +test_client_peer_reuse_failure_response2( + const struct http_response *resp, + struct _peer_reuse_failure *ctx) +{ + if (debug) + i_debug("RESPONSE: %u %s", resp->status, resp->reason); + + test_assert(resp->status == HTTP_CLIENT_REQUEST_ERROR_CONNECT_FAILED); + test_assert(resp->reason != NULL && *resp->reason != '\0'); + i_free(ctx); + io_loop_stop(ioloop); +} + +static void +test_client_peer_reuse_failure_next(struct _peer_reuse_failure *ctx) +{ + struct http_client_request *hreq; + + timeout_remove(&ctx->to); + + hreq = http_client_request(http_client, + "GET", net_ip2addr(&bind_ip), "/peer-reuse-next.txt", + test_client_peer_reuse_failure_response2, ctx); + http_client_request_set_port(hreq, bind_ports[0]); + http_client_request_submit(hreq); +} + +static void +test_client_peer_reuse_failure_response1( + const struct http_response *resp, + struct _peer_reuse_failure *ctx) +{ + if (debug) + i_debug("RESPONSE: %u %s", resp->status, resp->reason); + + if (ctx->first) { + test_assert(resp->status == 200); + + ctx->first = FALSE; + ctx->to = timeout_add_short(500, test_client_peer_reuse_failure_next, ctx); + } else { + test_assert(resp->status == HTTP_CLIENT_REQUEST_ERROR_CONNECT_FAILED); + } + + test_assert(resp->reason != NULL && *resp->reason != '\0'); +} + +static bool +test_client_peer_reuse_failure(const struct http_client_settings *client_set) +{ + struct http_client_request *hreq; + struct _peer_reuse_failure *ctx; + + ctx = i_new(struct _peer_reuse_failure, 1); + ctx->first = TRUE; + + http_client = http_client_init(client_set); + + hreq = http_client_request(http_client, + "GET", net_ip2addr(&bind_ip), "/peer-reuse.txt", + test_client_peer_reuse_failure_response1, ctx); + http_client_request_set_port(hreq, bind_ports[0]); + http_client_request_submit(hreq); + + hreq = http_client_request(http_client, + "GET", net_ip2addr(&bind_ip), "/peer-reuse.txt", + test_client_peer_reuse_failure_response1, ctx); + http_client_request_set_port(hreq, bind_ports[0]); + http_client_request_submit(hreq); + + hreq = http_client_request(http_client, + "GET", net_ip2addr(&bind_ip), "/peer-reuse.txt", + test_client_peer_reuse_failure_response1, ctx); + http_client_request_set_port(hreq, bind_ports[0]); + http_client_request_submit(hreq); + + return TRUE; +} + +/* test */ + +static void test_peer_reuse_failure(void) +{ + struct http_client_settings http_client_set; + + test_client_defaults(&http_client_set); + http_client_set.max_connect_attempts = 1; + http_client_set.max_idle_time_msecs = 500; + + test_begin("peer reuse failure"); + test_run_client_server(&http_client_set, + test_client_peer_reuse_failure, + test_server_peer_reuse_failure, 1, + NULL); + test_end(); +} + + /* * All tests */ @@ -2218,6 +2350,7 @@ static void (*test_functions[])(void) = { test_dns_timeout, test_dns_lookup_failure, test_dns_lookup_ttl, + test_peer_reuse_failure, NULL };