From 9e436d14399ab3bba5e302f1a4e67db92c942bf4 Mon Sep 17 00:00:00 2001 From: allengleyzer Date: Fri, 2 Feb 2018 17:56:48 -0800 Subject: [PATCH] Added progress updates for all XMLHttpRequest upload types / fix crash on closed connection Summary: This PR includes the same changes made in #16541, for addressing issues #11853/#15724. It adds upload progress updates for uploads with any request body type, and not just form-data. Additionally, this PR also includes a commit for fixing an `IllegalStateException` when a user's connection gets closed or times out (issues #10423/#11016). Since this exception was occurring within the progress updates logic, it started being thrown more frequently as a result of adding progress updates to all uploads, which was why the original PR was reverted. To test the upload progress updates, run the following JS to ensure events are now being dispatched: ``` const fileUri = 'file:///my_file.dat'; const url = 'http://my_post_url.com/'; const xhr = new XMLHttpRequest(); xhr.upload.onprogress = (event) => { console.log('progress: ' + event.loaded + ' / ' + event.total); } xhr.onreadystatechange = () => {if (xhr.readyState === 4) console.log('done');} console.log('start'); xhr.open('POST', url); // sending a file (wasn't sending progress) xhr.setRequestHeader('Content-Type', 'image/jpeg'); xhr.send({ uri: fileUri }); // sending a string (wasn't sending progress) xhr.setRequestHeader('Content-Type', 'text/plain'); xhr.send("some big string"); // sending form data (was already working) xhr.setRequestHeader('Content-Type', 'multipart/form-data'); const formData = new FormData(); formData.append('test', 'data'); xhr.send(formData); ``` To test the crash fix: In the RN Android project, before this change, set a breakpoint at `mRequestBody.writeTo(mBufferedSink);` of `ProgressRequestBody`, and wait a short while for a POST request with a non-null body to time out before resuming the app. Once resumed, if the connection was closed (the `closed` variable will be set to true in `RealBufferedSink`), an `IllegalStateException` will be thrown, which crashes the app. After the changes, an `IOException` will get thrown instead, which is already being properly handled. As mentioned above, includes the same changes as #16541, with an additional commit. [ANDROID] [BUGFIX] [XMLHttpRequest] - Added progress updates for all XMLHttpRequest upload types / fix crash on closed connection Previously, only form-data request bodies emitted upload progress updates. Now, other request body types will also emit updates. Also, Android will no longer crash on certain requests when user has a poor connection. Addresses issues: 11853/15724/10423/11016 Closes https://github.com/facebook/react-native/pull/17312 Differential Revision: D6712377 Pulled By: mdvacca fbshipit-source-id: bf5adc774703e7e66f7f16707600116f67201425 --- .../com/facebook/react/modules/network/BUCK | 1 + .../modules/network/NetworkingModule.java | 66 ++++++++++-------- .../modules/network/ProgressRequestBody.java | 69 +++++++++++-------- .../modules/network/NetworkingModuleTest.java | 9 ++- 4 files changed, 86 insertions(+), 59 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/BUCK b/ReactAndroid/src/main/java/com/facebook/react/modules/network/BUCK index 4ce467af9..16cd04306 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/network/BUCK +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/BUCK @@ -11,6 +11,7 @@ rn_android_library( ], deps = [ react_native_dep("libraries/fbcore/src/main/java/com/facebook/common/logging:logging"), + react_native_dep("libraries/fbcore/src/main/java/com/facebook/common/internal:internal"), react_native_dep("third-party/java/infer-annotations:infer-annotations"), react_native_dep("third-party/java/jsr-305:jsr-305"), react_native_dep("third-party/java/okhttp:okhttp3"), diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java b/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java index b94abebc0..fe379bb1f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java @@ -343,11 +343,11 @@ public final class NetworkingModule extends ReactContextBaseJavaModule { } } + RequestBody requestBody; if (data == null) { - requestBuilder.method(method, RequestBodyUtil.getEmptyBody(method)); + requestBody = RequestBodyUtil.getEmptyBody(method); } else if (handler != null) { - RequestBody requestBody = handler.toRequestBody(data, contentType); - requestBuilder.method(method, requestBody); + requestBody = handler.toRequestBody(data, contentType); } else if (data.hasKey(REQUEST_BODY_KEY_STRING)) { if (contentType == null) { ResponseUtil.onRequestError( @@ -360,14 +360,13 @@ public final class NetworkingModule extends ReactContextBaseJavaModule { String body = data.getString(REQUEST_BODY_KEY_STRING); MediaType contentMediaType = MediaType.parse(contentType); if (RequestBodyUtil.isGzipEncoding(contentEncoding)) { - RequestBody requestBody = RequestBodyUtil.createGzip(contentMediaType, body); + requestBody = RequestBodyUtil.createGzip(contentMediaType, body); if (requestBody == null) { ResponseUtil.onRequestError(eventEmitter, requestId, "Failed to gzip request body", null); return; } - requestBuilder.method(method, requestBody); } else { - requestBuilder.method(method, RequestBody.create(contentMediaType, body)); + requestBody = RequestBody.create(contentMediaType, body); } } else if (data.hasKey(REQUEST_BODY_KEY_BASE64)) { if (contentType == null) { @@ -380,9 +379,7 @@ public final class NetworkingModule extends ReactContextBaseJavaModule { } String base64String = data.getString(REQUEST_BODY_KEY_BASE64); MediaType contentMediaType = MediaType.parse(contentType); - requestBuilder.method( - method, - RequestBody.create(contentMediaType, ByteString.decodeBase64(base64String))); + requestBody = RequestBody.create(contentMediaType, ByteString.decodeBase64(base64String)); } else if (data.hasKey(REQUEST_BODY_KEY_URI)) { if (contentType == null) { ResponseUtil.onRequestError( @@ -403,9 +400,7 @@ public final class NetworkingModule extends ReactContextBaseJavaModule { null); return; } - requestBuilder.method( - method, - RequestBodyUtil.create(MediaType.parse(contentType), fileInputStream)); + requestBody = RequestBodyUtil.create(MediaType.parse(contentType), fileInputStream); } else if (data.hasKey(REQUEST_BODY_KEY_FORMDATA)) { if (contentType == null) { contentType = "multipart/form-data"; @@ -416,28 +411,16 @@ public final class NetworkingModule extends ReactContextBaseJavaModule { if (multipartBuilder == null) { return; } - - requestBuilder.method( - method, - RequestBodyUtil.createProgressRequest( - multipartBuilder.build(), - new ProgressListener() { - long last = System.nanoTime(); - - @Override - public void onProgress(long bytesWritten, long contentLength, boolean done) { - long now = System.nanoTime(); - if (done || shouldDispatch(now, last)) { - ResponseUtil.onDataSend(eventEmitter, requestId, bytesWritten, contentLength); - last = now; - } - } - })); + requestBody = multipartBuilder.build(); } else { // Nothing in data payload, at least nothing we could understand anyway. - requestBuilder.method(method, RequestBodyUtil.getEmptyBody(method)); + requestBody = RequestBodyUtil.getEmptyBody(method); } + requestBuilder.method( + method, + wrapRequestBodyWithProgressEmitter(requestBody, eventEmitter, requestId)); + addRequest(requestId); client.newCall(requestBuilder.build()).enqueue( new Callback() { @@ -515,6 +498,29 @@ public final class NetworkingModule extends ReactContextBaseJavaModule { }); } + private RequestBody wrapRequestBodyWithProgressEmitter( + final RequestBody requestBody, + final RCTDeviceEventEmitter eventEmitter, + final int requestId) { + if(requestBody == null) { + return null; + } + return RequestBodyUtil.createProgressRequest( + requestBody, + new ProgressListener() { + long last = System.nanoTime(); + + @Override + public void onProgress(long bytesWritten, long contentLength, boolean done) { + long now = System.nanoTime(); + if (done || shouldDispatch(now, last)) { + ResponseUtil.onDataSend(eventEmitter, requestId, bytesWritten, contentLength); + last = now; + } + } + }); + } + private void readWithProgress( RCTDeviceEventEmitter eventEmitter, int requestId, diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/ProgressRequestBody.java b/ReactAndroid/src/main/java/com/facebook/react/modules/network/ProgressRequestBody.java index 9676071aa..0611e2ffc 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/network/ProgressRequestBody.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/ProgressRequestBody.java @@ -9,60 +9,75 @@ package com.facebook.react.modules.network; +import com.facebook.common.internal.CountingOutputStream; + import java.io.IOException; + import okhttp3.MediaType; import okhttp3.RequestBody; import okio.BufferedSink; -import okio.Buffer; -import okio.Sink; -import okio.ForwardingSink; import okio.Okio; +import okio.Sink; public class ProgressRequestBody extends RequestBody { private final RequestBody mRequestBody; private final ProgressListener mProgressListener; private BufferedSink mBufferedSink; + private long mContentLength = 0L; public ProgressRequestBody(RequestBody requestBody, ProgressListener progressListener) { - mRequestBody = requestBody; - mProgressListener = progressListener; + mRequestBody = requestBody; + mProgressListener = progressListener; } @Override public MediaType contentType() { - return mRequestBody.contentType(); + return mRequestBody.contentType(); } @Override public long contentLength() throws IOException { - return mRequestBody.contentLength(); + if (mContentLength == 0) { + mContentLength = mRequestBody.contentLength(); + } + return mContentLength; } @Override public void writeTo(BufferedSink sink) throws IOException { - if (mBufferedSink == null) { - mBufferedSink = Okio.buffer(sink(sink)); - } - mRequestBody.writeTo(mBufferedSink); - mBufferedSink.flush(); + if (mBufferedSink == null) { + mBufferedSink = Okio.buffer(outputStreamSink(sink)); + } + + // contentLength changes for input streams, since we're using inputStream.available(), + // so get the length before writing to the sink + contentLength(); + + mRequestBody.writeTo(mBufferedSink); + mBufferedSink.flush(); } - private Sink sink(Sink sink) { - return new ForwardingSink(sink) { - long bytesWritten = 0L; - long contentLength = 0L; + private Sink outputStreamSink(BufferedSink sink) { + return Okio.sink(new CountingOutputStream(sink.outputStream()) { + @Override + public void write(byte[] data, int offset, int byteCount) throws IOException { + super.write(data, offset, byteCount); + sendProgressUpdate(); + } - @Override - public void write(Buffer source, long byteCount) throws IOException { - super.write(source, byteCount); - if (contentLength == 0) { - contentLength = contentLength(); - } - bytesWritten += byteCount; - mProgressListener.onProgress( - bytesWritten, contentLength, bytesWritten == contentLength); - } - }; + @Override + public void write(int data) throws IOException { + super.write(data); + sendProgressUpdate(); + } + + private void sendProgressUpdate() throws IOException { + long bytesWritten = getCount(); + long contentLength = contentLength(); + mProgressListener.onProgress( + bytesWritten, contentLength, bytesWritten == contentLength); + } + }); } } diff --git a/ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java b/ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java index 7b512bc25..8ad39cd16 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java @@ -200,6 +200,10 @@ public class NetworkingModuleTest { @Test public void testSuccessfulPostRequest() throws Exception { + RCTDeviceEventEmitter emitter = mock(RCTDeviceEventEmitter.class); + ReactApplicationContext context = mock(ReactApplicationContext.class); + when(context.getJSModule(any(Class.class))).thenReturn(emitter); + OkHttpClient httpClient = mock(OkHttpClient.class); when(httpClient.newCall(any(Request.class))).thenAnswer(new Answer() { @Override @@ -211,12 +215,13 @@ public class NetworkingModuleTest { OkHttpClient.Builder clientBuilder = mock(OkHttpClient.Builder.class); when(clientBuilder.build()).thenReturn(httpClient); when(httpClient.newBuilder()).thenReturn(clientBuilder); - NetworkingModule networkingModule = - new NetworkingModule(mock(ReactApplicationContext.class), "", httpClient); + NetworkingModule networkingModule = new NetworkingModule(context, "", httpClient); JavaOnlyMap body = new JavaOnlyMap(); body.putString("string", "This is request body"); + mockEvents(); + networkingModule.sendRequest( "POST", "http://somedomain/bar",