From f836754b6e2c80a167c3243682a2f387c90d8c22 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Mon, 25 Nov 2019 09:28:16 +0100 Subject: [PATCH] Improve Stability of Mock APIs (#49518) This commit ensures that even for requests that are known to be empty body we at least attempt to read one bytes from the request body input stream. This is done to work around the behavior in `sun.net.httpserver.ServerImpl.Dispatcher#handleEvent` that will close a TCP/HTTP connection that does not have the `eof` flag (see `sun.net.httpserver.LeftOverInputStream#isEOF`) set on its input stream. As far as I can tell the only way to set this flag is to do a read when there's no more bytes buffered. This fixes the numerous connection closing issues because the `ServerImpl` stops closing connections that it thinks weren't fully drained. Also, I removed a now redundant drain loop in the Azure handler as well as removed the connection closing in the error handler's drain action (this shouldn't have an effect but makes things more predictable/easier to reason about IMO). I would suggest merging this and closing related issue after verifying that this fixes things on CI. The way to locally reproduce the issues we're seeing in tests is to make the retry timings more aggressive in e.g. the azure tests and move them to single digit values. This makes the retries happen quickly enough that they run into the async connecting closing of allegedly non-eof connections by `ServerImpl` and produces the exact kinds of failures we're seeing currently. Relates #49401, #49429 --- .../src/main/java/fixture/azure/AzureHttpHandler.java | 8 ++++---- .../java/fixture/gcs/GoogleCloudStorageHttpHandler.java | 4 ++++ .../src/main/java/fixture/s3/S3HttpHandler.java | 4 ++++ .../blobstore/ESMockAPIBasedRepositoryIntegTestCase.java | 4 +--- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpHandler.java b/test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpHandler.java index 902aa4f132eb2..7a94a8c9f2e57 100644 --- a/test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpHandler.java +++ b/test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpHandler.java @@ -31,7 +31,6 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; -import java.io.InputStream; import java.io.InputStreamReader; import java.nio.charset.StandardCharsets; import java.util.Arrays; @@ -63,6 +62,10 @@ public AzureHttpHandler(final String container) { @Override public void handle(final HttpExchange exchange) throws IOException { final String request = exchange.getRequestMethod() + " " + exchange.getRequestURI().toString(); + if (request.startsWith("GET") || request.startsWith("HEAD") || request.startsWith("DELETE")) { + int read = exchange.getRequestBody().read(); + assert read == -1 : "Request body should have been empty but saw [" + read + "]"; + } try { if (Regex.simpleMatch("PUT /" + container + "/*blockid=*", request)) { // Put Block (https://docs.microsoft.com/en-us/rest/api/storageservices/put-block) @@ -140,9 +143,6 @@ public void handle(final HttpExchange exchange) throws IOException { } else if (Regex.simpleMatch("DELETE /" + container + "/*", request)) { // Delete Blob (https://docs.microsoft.com/en-us/rest/api/storageservices/delete-blob) - try (InputStream is = exchange.getRequestBody()) { - while (is.read() >= 0); - } blobs.entrySet().removeIf(blob -> blob.getKey().startsWith(exchange.getRequestURI().getPath())); exchange.sendResponseHeaders(RestStatus.ACCEPTED.getStatus(), -1); diff --git a/test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java b/test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java index 2829341139e4c..6c007eb3734de 100644 --- a/test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java +++ b/test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java @@ -76,6 +76,10 @@ public GoogleCloudStorageHttpHandler(final String bucket) { @Override public void handle(final HttpExchange exchange) throws IOException { final String request = exchange.getRequestMethod() + " " + exchange.getRequestURI().toString(); + if (request.startsWith("GET") || request.startsWith("HEAD") || request.startsWith("DELETE")) { + int read = exchange.getRequestBody().read(); + assert read == -1 : "Request body should have been empty but saw [" + read + "]"; + } try { if (Regex.simpleMatch("GET /storage/v1/b/" + bucket + "/o*", request)) { // List Objects https://cloud.google.com/storage/docs/json_api/v1/objects/list diff --git a/test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpHandler.java b/test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpHandler.java index f4a467d314a27..f9bce9f02c85d 100644 --- a/test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpHandler.java +++ b/test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpHandler.java @@ -79,6 +79,10 @@ public S3HttpHandler(final String bucket, @Nullable final String basePath) { @Override public void handle(final HttpExchange exchange) throws IOException { final String request = exchange.getRequestMethod() + " " + exchange.getRequestURI().toString(); + if (request.startsWith("GET") || request.startsWith("HEAD") || request.startsWith("DELETE")) { + int read = exchange.getRequestBody().read(); + assert read == -1 : "Request body should have been empty but saw [" + read + "]"; + } try { if (Regex.simpleMatch("POST /" + path + "/*?uploads", request)) { final String uploadId = UUIDs.randomBase64UUID(); diff --git a/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/ESMockAPIBasedRepositoryIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/ESMockAPIBasedRepositoryIntegTestCase.java index 74b126efe574a..040961ed52b1d 100644 --- a/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/ESMockAPIBasedRepositoryIntegTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/ESMockAPIBasedRepositoryIntegTestCase.java @@ -162,9 +162,7 @@ protected static String httpServerUrl() { * Consumes and closes the given {@link InputStream} */ protected static void drainInputStream(final InputStream inputStream) throws IOException { - try (InputStream is = inputStream) { - while (is.read(BUFFER) >= 0); - } + while (inputStream.read(BUFFER) >= 0) ; } /**