From 02bf39966ad58d8369ecbdb1a4c4384cbdef8fe2 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 31 Oct 2018 10:03:06 -0700 Subject: [PATCH 1/5] Delay request initialization for resumable upload until the content chunk is ready --- .../googleapis/media/MediaHttpUploader.java | 35 +++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java b/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java index 5fe874557..21f8a0e97 100644 --- a/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java +++ b/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java @@ -405,8 +405,11 @@ private HttpResponse resumableUpload(GenericUrl initiationRequestUrl) throws IOE HttpResponse response; // Upload the media content in chunks. while (true) { + ContentChunk contentChunk = buildContentChunk(); currentRequest = requestFactory.buildPutRequest(uploadUrl, null); - setContentAndHeadersOnCurrentRequest(); + currentRequest.setContent(contentChunk.getContent()); + currentRequest.getHeaders().setContentRange(contentChunk.getContentRange()); + // set mediaErrorHandler as I/O exception handler and as unsuccessful response handler for // calling to serverErrorCallback on an I/O exception or an abnormal HTTP response new MediaUploadErrorHandler(this, currentRequest); @@ -567,7 +570,7 @@ private HttpResponse executeCurrentRequest(HttpRequest request) throws IOExcepti * Sets the HTTP media content chunk and the required headers that should be used in the upload * request. */ - private void setContentAndHeadersOnCurrentRequest() throws IOException { + private ContentChunk buildContentChunk() throws IOException { int blockSize; if (isMediaLengthKnown()) { // We know exactly what the blockSize will be because we know the media content length. @@ -652,15 +655,35 @@ private void setContentAndHeadersOnCurrentRequest() throws IOException { } currentChunkLength = actualBlockSize; - currentRequest.setContent(contentChunk); + + String contentRange; if (actualBlockSize == 0) { // No bytes to upload. Either zero content media being uploaded, or a server failure on the // last write, even though the write actually succeeded. Either way, // mediaContentLengthStr will contain the actual media length. - currentRequest.getHeaders().setContentRange("bytes */" + mediaContentLengthStr); + contentRange = "bytes */" + mediaContentLengthStr; } else { - currentRequest.getHeaders().setContentRange("bytes " + totalBytesServerReceived + "-" - + (totalBytesServerReceived + actualBlockSize - 1) + "/" + mediaContentLengthStr); + contentRange = "bytes " + totalBytesServerReceived + "-" + + (totalBytesServerReceived + actualBlockSize - 1) + "/" + mediaContentLengthStr; + } + return new ContentChunk(contentChunk, contentRange); + } + + private class ContentChunk { + private AbstractInputStreamContent content; + private String contentRange; + + ContentChunk(AbstractInputStreamContent content, String contentRange) { + this.content = content; + this.contentRange = contentRange; + } + + AbstractInputStreamContent getContent() { + return content; + } + + String getContentRange() { + return contentRange; } } From aae57230a98daa1f398ff82d074092562282efc5 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 31 Oct 2018 11:20:21 -0700 Subject: [PATCH 2/5] Fix javadoc link --- .../google/api/client/googleapis/media/MediaHttpUploader.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java b/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java index 21f8a0e97..74ad8c270 100644 --- a/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java +++ b/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java @@ -246,7 +246,7 @@ public enum UploadState { /** * The content buffer of the current request or {@code null} for none. It is used for resumable * media upload when the media content length is not specified. It is instantiated for every - * request in {@link #setContentAndHeadersOnCurrentRequest} and is set to {@code null} when the + * request in {@link #buildContentChunk()} and is set to {@code null} when the * request is completed in {@link #upload}. */ private byte currentRequestContentBuffer[]; From 14b72a64d4b3be2adeb7e405891974ae6b7ba545 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 31 Oct 2018 13:26:54 -0700 Subject: [PATCH 3/5] Adding test for a slow writer --- .../media/MediaHttpUploaderTest.java | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/google-api-client/src/test/java/com/google/api/client/googleapis/media/MediaHttpUploaderTest.java b/google-api-client/src/test/java/com/google/api/client/googleapis/media/MediaHttpUploaderTest.java index aadfe7bae..6feff227b 100644 --- a/google-api-client/src/test/java/com/google/api/client/googleapis/media/MediaHttpUploaderTest.java +++ b/google-api-client/src/test/java/com/google/api/client/googleapis/media/MediaHttpUploaderTest.java @@ -39,6 +39,8 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.io.PipedInputStream; +import java.io.PipedOutputStream; import java.util.Arrays; import java.util.Random; import java.util.logging.Level; @@ -1103,4 +1105,68 @@ public void testResumableMediaUploadWithoutContentClose() throws Exception { uploader.upload(new GenericUrl(TEST_RESUMABLE_REQUEST_URL)); assertFalse(inputStream.isClosed()); } + + class SlowWriter implements Runnable { + final private OutputStream outputStream; + final private int contentLength; + + SlowWriter(OutputStream outputStream, int contentLength) { + this.outputStream = outputStream; + this.contentLength = contentLength; + } + + @Override + public void run() { + try { + for (int i = 0; i < contentLength; i++) { + outputStream.write(i); + Thread.sleep(1000); + } + outputStream.close(); + } catch (IOException e) { + // ignore + } catch (InterruptedException e) { + // ignore + } + } + } + + class TimeoutRequestInitializer implements HttpRequestInitializer { + class TimingInterceptor implements HttpExecuteInterceptor { + private long initTime; + + TimingInterceptor() { + initTime = System.currentTimeMillis(); + } + + @Override + public void intercept(HttpRequest request) { + assertTrue( + "Request initialization to execute should be fast", + System.currentTimeMillis() - initTime < 100L + ); + } + } + + @Override + public void initialize(HttpRequest request) { + request.setInterceptor(new TimingInterceptor()); + } + } + + public void testResumableSlowUpload() throws Exception { + int contentLength = 3; + MediaTransport fakeTransport = new MediaTransport(contentLength); + fakeTransport.contentLengthNotSpecified = true; + PipedOutputStream outputStream = new PipedOutputStream(); + InputStream inputStream = new PipedInputStream(outputStream); + + Thread thread = new Thread(new SlowWriter(outputStream, contentLength)); + thread.start(); + + InputStreamContent mediaContent = new InputStreamContent(TEST_CONTENT_TYPE, inputStream); + MediaHttpUploader uploader = new MediaHttpUploader(mediaContent, fakeTransport, new TimeoutRequestInitializer()); + uploader.setDirectUploadEnabled(false); + uploader.upload(new GenericUrl(TEST_RESUMABLE_REQUEST_URL)); + } } From c37214ca1aef248c52468c8e1788507c5741528d Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Fri, 2 Nov 2018 09:21:42 -0700 Subject: [PATCH 4/5] Make ContentChunk class static for internal clarity lint. --- .../google/api/client/googleapis/media/MediaHttpUploader.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java b/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java index 74ad8c270..e8565852a 100644 --- a/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java +++ b/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java @@ -669,7 +669,7 @@ private ContentChunk buildContentChunk() throws IOException { return new ContentChunk(contentChunk, contentRange); } - private class ContentChunk { + private static class ContentChunk { private AbstractInputStreamContent content; private String contentRange; From cdf5db6cfcead4fe78f590a265a8eb20a3f50dc2 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Fri, 2 Nov 2018 09:25:32 -0700 Subject: [PATCH 5/5] Add final to ContentChunk fields set only in constructor --- .../google/api/client/googleapis/media/MediaHttpUploader.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java b/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java index e8565852a..7b8e8da40 100644 --- a/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java +++ b/google-api-client/src/main/java/com/google/api/client/googleapis/media/MediaHttpUploader.java @@ -670,8 +670,8 @@ private ContentChunk buildContentChunk() throws IOException { } private static class ContentChunk { - private AbstractInputStreamContent content; - private String contentRange; + private final AbstractInputStreamContent content; + private final String contentRange; ContentChunk(AbstractInputStreamContent content, String contentRange) { this.content = content;