From 4eb2391f298ecea89345701aa17dc772189980c7 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 26 Sep 2018 13:29:01 -0700 Subject: [PATCH 1/7] Introduce API to set writeTimeout and use it in the NetHttp request --- .../api/client/http/LowLevelHttpRequest.java | 15 +++++ .../client/http/javanet/NetHttpRequest.java | 56 ++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/LowLevelHttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/LowLevelHttpRequest.java index a3f489efc..d3900f91c 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/LowLevelHttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/LowLevelHttpRequest.java @@ -159,6 +159,21 @@ public final StreamingContent getStreamingContent() { public void setTimeout(int connectTimeout, int readTimeout) throws IOException { } + /** + * Sets the write timeout for POST/PUT requests. + * + *

+ * Default implementation does nothing, but subclasses should normally override. + *

+ * + * @param writeTimeout timeout in milliseconds to establish a connection or {@code 0} for an + * infinite timeout + * @throws IOException I/O exception + * @since 1.26 + */ + public void setWriteTimeout(int writeTimeout) throws IOException { + } + /** Executes the request and returns a low-level HTTP response object. */ public abstract LowLevelHttpResponse execute() throws IOException; } diff --git a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java index d0d8d0cb1..47292cb94 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java @@ -18,9 +18,18 @@ import com.google.api.client.http.LowLevelHttpResponse; import com.google.api.client.util.Preconditions; +import com.google.api.client.util.StreamingContent; import java.io.IOException; import java.io.OutputStream; import java.net.HttpURLConnection; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.FutureTask; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; /** * @author Yaniv Inbar @@ -28,12 +37,14 @@ final class NetHttpRequest extends LowLevelHttpRequest { private final HttpURLConnection connection; + private int writeTimeout; /** * @param connection HTTP URL connection */ NetHttpRequest(HttpURLConnection connection) { this.connection = connection; + this.writeTimeout = 0; connection.setInstanceFollowRedirects(false); } @@ -48,8 +59,14 @@ public void setTimeout(int connectTimeout, int readTimeout) { connection.setConnectTimeout(connectTimeout); } + @Override + public void setWriteTimeout(int writeTimeout) throws IOException { + this.writeTimeout = writeTimeout; + } + @Override public LowLevelHttpResponse execute() throws IOException { + System.out.println("in execute"); HttpURLConnection connection = this.connection; // write content if (getStreamingContent() != null) { @@ -77,7 +94,7 @@ public LowLevelHttpResponse execute() throws IOException { OutputStream out = connection.getOutputStream(); boolean threw = true; try { - getStreamingContent().writeTo(out); + writeContentToOutputStream(out); threw = false; } finally { try { @@ -101,7 +118,9 @@ public LowLevelHttpResponse execute() throws IOException { // connect boolean successfulConnection = false; try { + System.out.println("before connect"); connection.connect(); + System.out.println("after connect"); NetHttpResponse response = new NetHttpResponse(connection); successfulConnection = true; return response; @@ -111,4 +130,39 @@ public LowLevelHttpResponse execute() throws IOException { } } } + + private void writeContentToOutputStream(OutputStream out) + throws IOException { + if (writeTimeout == 0) { + // If no timeout set, do a plain write + getStreamingContent().writeTo(out); + } else { + // Use and executor service and futures to handle this + final OutputStream outputStream = out; + final StreamingContent content = getStreamingContent(); + final Callable writeContent = new Callable() { + @Override + public Boolean call() throws IOException { + content.writeTo(outputStream); + return Boolean.TRUE; + } + }; + final ExecutorService executor = Executors.newSingleThreadExecutor(); + final Future future = executor.submit(new FutureTask(writeContent)); + executor.shutdown(); + + try { + future.get(writeTimeout, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + throw new IOException("Socket write interrupted", e); + } catch (ExecutionException e) { + throw new IOException("Exception in socket write", e); + } catch (TimeoutException e) { + throw new IOException("Socket write timed out", e); + } + if (!executor.isTerminated()) { + executor.shutdown(); + } + } + } } From 6f2122afa25347e4c784b7a2e904f6956d2c3d8f Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 26 Sep 2018 15:41:58 -0700 Subject: [PATCH 2/7] Surface the writeTimeout to HttpRequest --- .../google/api/client/http/HttpRequest.java | 30 +++++++++++++++++++ pom.xml | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java index 6376b2b7f..566c25aa3 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java @@ -158,6 +158,11 @@ static String executeAndGetValueOfSomeCustomHeader(HttpRequest request) { */ private int readTimeout = 20 * 1000; + /** + * Timeout in milliseconds to set POST/PUT data or {@code 0} for an infinite timeout. + */ + private int writeTimeout = 0; + /** HTTP unsuccessful (non-2XX) response handler or {@code null} for none. */ private HttpUnsuccessfulResponseHandler unsuccessfulResponseHandler; @@ -493,6 +498,30 @@ public HttpRequest setReadTimeout(int readTimeout) { return this; } + /** + * Returns the timeout in milliseconds to send POST/PUT data or {@code 0} for an infinite timeout. + * + *

+ * By default it is 0 (infinite). + *

+ * + * @since 1.26 + */ + public int getWriteTimeout() { + return writeTimeout; + } + + /** + * Sets the timeout in milliseconds to send POST/PUT data or {@code 0} for an infinite timeout. + * + * @since 1.26 + */ + public HttpRequest setWriteTimeout(int writeTimeout) { + Preconditions.checkArgument(writeTimeout >= 0); + this.writeTimeout = writeTimeout; + return this; + } + /** * Returns the HTTP request headers. * @@ -977,6 +1006,7 @@ public HttpResponse execute() throws IOException { // execute lowLevelHttpRequest.setTimeout(connectTimeout, readTimeout); + lowLevelHttpRequest.setWriteTimeout(writeTimeout); try { LowLevelHttpResponse lowLevelHttpResponse = lowLevelHttpRequest.execute(); // Flag used to indicate if an exception is thrown before the response is constructed. diff --git a/pom.xml b/pom.xml index 665e4d9b1..1be3f1209 100644 --- a/pom.xml +++ b/pom.xml @@ -526,7 +526,7 @@ org.codehaus.mojo.signature - java15 + java16 1.0 From 8b29f5dc5181511d6ecbd8b4e4c6c9f356a494d5 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 3 Oct 2018 18:09:33 -0700 Subject: [PATCH 3/7] remove debug statements --- .../com/google/api/client/http/javanet/NetHttpRequest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java index 47292cb94..2a3a15832 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java @@ -66,7 +66,6 @@ public void setWriteTimeout(int writeTimeout) throws IOException { @Override public LowLevelHttpResponse execute() throws IOException { - System.out.println("in execute"); HttpURLConnection connection = this.connection; // write content if (getStreamingContent() != null) { @@ -118,9 +117,7 @@ public LowLevelHttpResponse execute() throws IOException { // connect boolean successfulConnection = false; try { - System.out.println("before connect"); connection.connect(); - System.out.println("after connect"); NetHttpResponse response = new NetHttpResponse(connection); successfulConnection = true; return response; From 89e3e68391ee608f3a0b1d729fe660d55b9c54e0 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Thu, 4 Oct 2018 14:26:22 -0700 Subject: [PATCH 4/7] Fix warnings --- .../com/google/api/client/http/javanet/NetHttpRequest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java index 2a3a15832..ac7676f3b 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java @@ -137,7 +137,7 @@ private void writeContentToOutputStream(OutputStream out) // Use and executor service and futures to handle this final OutputStream outputStream = out; final StreamingContent content = getStreamingContent(); - final Callable writeContent = new Callable() { + final Callable writeContent = new Callable() { @Override public Boolean call() throws IOException { content.writeTo(outputStream); @@ -145,7 +145,7 @@ public Boolean call() throws IOException { } }; final ExecutorService executor = Executors.newSingleThreadExecutor(); - final Future future = executor.submit(new FutureTask(writeContent)); + final Future future = executor.submit(new FutureTask(writeContent)); executor.shutdown(); try { From e3bd6f13367d50f63408949dcf3559eafb20f487 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Thu, 4 Oct 2018 14:38:33 -0700 Subject: [PATCH 5/7] Fix syntax --- .../java/com/google/api/client/http/javanet/NetHttpRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java index ac7676f3b..b63df00f1 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java @@ -145,7 +145,7 @@ public Boolean call() throws IOException { } }; final ExecutorService executor = Executors.newSingleThreadExecutor(); - final Future future = executor.submit(new FutureTask(writeContent)); + final Future future = executor.submit(new FutureTask(writeContent), null); executor.shutdown(); try { From 8d91756b33cd5990289ccfd830aae048f159b9a4 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Thu, 4 Oct 2018 16:45:50 -0700 Subject: [PATCH 6/7] DI for testing --- .../client/http/javanet/NetHttpRequest.java | 39 +++++++-- .../http/javanet/NetHttpRequestTest.java | 85 +++++++++++++++++++ .../src/test/resources/file.txt | 1 + 3 files changed, 116 insertions(+), 9 deletions(-) create mode 100644 google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpRequestTest.java create mode 100644 google-http-client/src/test/resources/file.txt diff --git a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java index b63df00f1..5ff66a8b2 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java @@ -19,6 +19,7 @@ import com.google.api.client.util.Preconditions; import com.google.api.client.util.StreamingContent; +import com.google.common.annotations.VisibleForTesting; import java.io.IOException; import java.io.OutputStream; import java.net.HttpURLConnection; @@ -64,8 +65,27 @@ public void setWriteTimeout(int writeTimeout) throws IOException { this.writeTimeout = writeTimeout; } + interface OutputWriter { + void write(OutputStream outputStream, StreamingContent content) throws IOException; + } + + static class DefaultOutputWriter implements OutputWriter { + @Override + public void write(OutputStream outputStream, final StreamingContent content) + throws IOException { + content.writeTo(outputStream); + } + } + + private static OutputWriter DEFAULT_CONNECTION_WRITER = new DefaultOutputWriter(); + @Override public LowLevelHttpResponse execute() throws IOException { + return execute(DEFAULT_CONNECTION_WRITER); + } + + @VisibleForTesting + LowLevelHttpResponse execute(final OutputWriter outputWriter) throws IOException { HttpURLConnection connection = this.connection; // write content if (getStreamingContent() != null) { @@ -90,10 +110,12 @@ public LowLevelHttpResponse execute() throws IOException { } else { connection.setChunkedStreamingMode(0); } - OutputStream out = connection.getOutputStream(); + final OutputStream out = connection.getOutputStream(); + boolean threw = true; try { - writeContentToOutputStream(out); + writeContentToOutputStream(outputWriter, out); + threw = false; } finally { try { @@ -128,24 +150,23 @@ public LowLevelHttpResponse execute() throws IOException { } } - private void writeContentToOutputStream(OutputStream out) + private void writeContentToOutputStream(final OutputWriter outputWriter, final OutputStream out) throws IOException { if (writeTimeout == 0) { - // If no timeout set, do a plain write - getStreamingContent().writeTo(out); + outputWriter.write(out, getStreamingContent()); } else { - // Use and executor service and futures to handle this - final OutputStream outputStream = out; + // do it with timeout final StreamingContent content = getStreamingContent(); final Callable writeContent = new Callable() { @Override public Boolean call() throws IOException { - content.writeTo(outputStream); + outputWriter.write(out, content); return Boolean.TRUE; } }; + final ExecutorService executor = Executors.newSingleThreadExecutor(); - final Future future = executor.submit(new FutureTask(writeContent), null); + final Future future = executor.submit(new FutureTask(writeContent), null); executor.shutdown(); try { diff --git a/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpRequestTest.java b/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpRequestTest.java new file mode 100644 index 000000000..6839dfd76 --- /dev/null +++ b/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpRequestTest.java @@ -0,0 +1,85 @@ +package com.google.api.client.http.javanet; + +import com.google.api.client.http.HttpContent; +import com.google.api.client.http.InputStreamContent; +import com.google.api.client.http.javanet.NetHttpRequest.OutputWriter; +import com.google.api.client.testing.http.HttpTesting; +import com.google.api.client.testing.http.javanet.MockHttpURLConnection; +import com.google.api.client.util.StreamingContent; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.URL; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.util.concurrent.TimeoutException; +import org.junit.Test; + +public class NetHttpRequestTest { + + static class SleepingOutputWriter implements OutputWriter { + private long sleepTimeInMs; + public SleepingOutputWriter(long sleepTimeInMs) { + this.sleepTimeInMs = sleepTimeInMs; + } + @Override + public void write(OutputStream outputStream, StreamingContent content) throws IOException { + try { + Thread.sleep(sleepTimeInMs); + } catch (InterruptedException e) { + throw new IOException("sleep interrupted", e); + } + } + } + + @Test + public void testHangingWrite() throws InterruptedException { + Thread thread = new Thread() { + @Override + public void run() { + try { + postWithTimeout(0); + } catch (IOException e) { + // expected to be interrupted + assertEquals(e.getCause().getClass(), InterruptedException.class); + return; + } catch (Exception e) { + fail(); + } + fail("should be interrupted before here"); + } + }; + + thread.start(); + Thread.sleep(1000); + assertTrue(thread.isAlive()); + thread.interrupt(); + } + + @Test(timeout = 1000) + public void testOutputStreamWriteTimeout() throws Exception { + try { + postWithTimeout(100); + fail("should have timed out"); + } catch (IOException e) { + assertEquals(e.getCause().getClass(), TimeoutException.class); + } catch (Exception e) { + fail("Expected an IOException not a " + e.getCause().getClass().getName()); + } + } + + private static void postWithTimeout(int timeout) throws Exception { + MockHttpURLConnection connection = new MockHttpURLConnection(new URL(HttpTesting.SIMPLE_URL)); + connection.setRequestMethod("POST"); + NetHttpRequest request = new NetHttpRequest(connection); + InputStream is = NetHttpRequestTest.class.getClassLoader().getResourceAsStream("file.txt"); + HttpContent content = new InputStreamContent("text/plain", is); + request.setStreamingContent(content); + request.setWriteTimeout(timeout); + request.execute(new SleepingOutputWriter(5000L)); + } + +} diff --git a/google-http-client/src/test/resources/file.txt b/google-http-client/src/test/resources/file.txt new file mode 100644 index 000000000..1065c29e7 --- /dev/null +++ b/google-http-client/src/test/resources/file.txt @@ -0,0 +1 @@ +some sample file From 93ab5f9bbe71050b543ddfe7f17fb3e1e15cb08f Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Tue, 6 Nov 2018 15:45:53 -0800 Subject: [PATCH 7/7] Make field constant field final --- .../java/com/google/api/client/http/javanet/NetHttpRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java index 5ff66a8b2..cd92cac11 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java @@ -77,7 +77,7 @@ public void write(OutputStream outputStream, final StreamingContent content) } } - private static OutputWriter DEFAULT_CONNECTION_WRITER = new DefaultOutputWriter(); + private static final OutputWriter DEFAULT_CONNECTION_WRITER = new DefaultOutputWriter(); @Override public LowLevelHttpResponse execute() throws IOException {