Skip to content

Commit

Permalink
Improve Stability of Mock APIs (#49518) (#49525)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
original-brownbear authored Nov 25, 2019
1 parent 3a96f4e commit 732fc4d
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) ;
}

/**
Expand Down

0 comments on commit 732fc4d

Please sign in to comment.