[Android] Added progress updates for all XMLHttpRequest upload types / fix crash on closed connection

[allengleyzer]

Motivation

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.

Test Plan

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.

Related PRs

As mentioned above, includes the same changes as #16541, with an additional commit.

Release Notes

[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

[allengleyzer]

One note on the changes is that only file uploads actually provide regular updates for the progress (which is probably the most common use case, though). String, form-data, gzip, and base64 request bodies all seem to just send a single progress update at the very end (even despite form-data being the only one that used to send updates at all).

This is a result of Okio's RealBufferedSink implementation of write(bytes[]), which writes all its data in a single pass.

Some possible solutions are to implement a decorator BufferedSink class and change the write(bytes[]) method, or to create a custom RequestBody class whose writeTo method writes the data in chunks, or to convert the byte arrays to an InputStream, and create the RequestBody using the method that takes in an input stream. However, I'm not sure whether those changes are in the scope of this PR, nor that I'm a good candidate to choose the ideal approach.

[allengleyzer]

On second thought, replacing the ForwardingSink with an sink wrapping an OutputStream solves both the crashing (without necessitating a try-catch), and fixes the progress updates for the other types. PR updated.

[LEEY19]

@allengleyzer bringing our discussion over from #16541 (comment)

I have ensured that Content-Type header is set with the correct content type, but there is still nothing when xhr.upload.onprogress is called. I am modifying the code base from RN 0.47.2. Could that be the reason?

[allengleyzer]

@LEEY19 I'm not sure, I've only tested on 0.48.x and later versions, but I don't think it would make a difference, since this area of the code hasn't seen many (recent) changes.

Have you tried the newer changes in this PR (as opposed to the previous)? Also, what type of data are you trying to upload?

[janicduplessis]

@allengleyzer Nice work, I looked at an earlier version and it's great you managed to find a solution that works without the try-catch.

[LEEY19]

@allengleyzer I have ensured that I copy-paste exactly all the 3 files that are affected in this PR. There is still nothing.. my code is as below:

    API.post('/uploads/sign', token, {name: message.name, type: contentType, size: file.fileSize ? file.fileSize : null})
    .then(response => {

      let xhr = new XMLHttpRequest()

      xhr.upload.onprogress = (evt) => {
        console.log('progress: ' + evt.loaded + ' / ' + evt.total);
        // if (evt.lengthComputable) {
        //   dispatch({type: types.UPDATE_FILE_UPLOAD, channel_id: channelId, id: message.id, uploadProgress: evt.loaded/evt.total })
        // } else {
        //   console.log("Cannot get upload progress")
        // }
      }
      xhr.onreadystatechange = () => {
        if(xhr.readyState === 4 && xhr.status === 200) {
          message.uploadProgress = 'complete'

          if(message.type.includes("image")) {
            message.image = response.url
          } else if (message.type.includes("video")) {
            message.video = response.url
          } else {
            message.file = response.url
          }

          dispatch({type: types.UPDATE_FILE_UPLOAD, channel_id: channelId, id: message.id, message })
          channel.push("new:message", message)
        } else {
          console.log("Error: PUT /uploads/sign", xhr.response)
        }
      }
      xhr.open("PUT", response.signed_url)
      xhr.setRequestHeader('Content-Type', contentType)
      xhr.send(file)

      dispatch({type: types.SEND_FILE, message, channel_id: channelId })


      xhr.upload.onerror = (evt) => {
        dispatch({type: types.UPDATE_FILE_UPLOAD, channel_id: channelId, id: message.id, uploadProgress: 'failed' })
      }

    }).catch(error => {
      console.log("Error: POST /uploads/sign", error)
    })

The file does get sent through ultimately, and xhr.onreadystatechange works as well. Just that console.log('progress: ' + evt.loaded + ' / ' + evt.total); is still not triggered :(

[janicduplessis]

@LEEY19 Are you building from source (http://facebook.github.io/react-native/docs/android-building-from-source.html), on Android we use prebuilt artifacts by default so just changing the source doesn't have any effect.

[janicduplessis]

@allengleyzer Will merge this after holidays!

[derakhshanfar]

hi @janicduplessis , when do you merge it?

[derakhshanfar]

@janicduplessis @allengleyzer thanks so much but how can i upgrade react-native that refer to specific commit or tag when it is not release on NPM?

[LEEY19]

@janicduplessis sorry my bad. I had my own RN fork as I had implemented some of my own native code changes before. But when I copy-pasted @allengleyzer 's code I forgot to repackage the required android files https://facebook.github.io/react-native/docs/android-building-from-source.html#building-for-maven-nexus-deployment

Once that is done the code works, thanks guys! Great work 👍

[allengleyzer]

Thanks @janicduplessis, I wasn't crazy about it either, though to be fair, there probably isn't much of a difference in the end result, since it's more of a implementation detail that by throwing an IOException, it is already being handled (by essentially doing nothing), whereas before, the IllegalStateException went unhandled. Nonetheless, it certainly is cleaner this way, and it's nice that it also fixes the progress issues for the other upload types.

@LEEY19 Glad you got it working.

@derakhshanfar You can create a fork of the release you're using, cherry pick/copy these commits, and reference your branch in your package.json. You'll need to build the ReactAndroid libraries using ./gradlew ReactAndroid:installArchives, so that's something you can add to your build steps. For full details: https://facebook.github.io/react-native/docs/android-building-from-source.html

[facebook-github-bot]

@mdvacca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[janicduplessis]

@mdvacca Any update on this?

[facebook-github-bot]

@allengleyzer I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[allengleyzer]

Rebased the PR.

The only change made from the previous iteration was to have requests made via a RequestBodyHandler also get wrapped with progress updates, since that was recently introduced.

[janicduplessis]

Thanks @allengleyzer, I'll try shipping again since I haven't had news from @mdvacca.

@facebook-github-bot shipit

[facebook-github-bot]

@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[hramos]

This won’t land due to a missing buck dep internally but @mdvacca seems to be aware of what’s needed. I can take a look Monday.

[facebook-github-bot]

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

[mdvacca]

I just fixed the dependency error and I'm landing it

[vvusts]

Is this fix part of RN .53?

[hramos]

You can check if this is in a given release by looking at the tags on the commit itself.