Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix support for blobs larger than 64 KB on Android #31789

Closed
wants to merge 9 commits into from
Closed

Fix support for blobs larger than 64 KB on Android #31789

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1b92132
Register `BlobProvider` as a ContentProvider
tomekzaw Jun 29, 2021
2116f50
Add blob object URL as Image component source example in RNTester app
tomekzaw Jun 29, 2021
2ffbd5a
Fix `BlobProvider.openFile` for blobs larger than pipe capacity
tomekzaw Jun 29, 2021
57e3173
Declare `data` and `writeSide` final
tomekzaw Jun 29, 2021
005d4a6
Fix Flow errors
tomekzaw Jun 29, 2021
82d3284
Fix code style
tomekzaw Jun 29, 2021
4b886e2
Conditionally create new thread
tomekzaw Jul 14, 2021
58c8354
Fix code style
tomekzaw Jul 14, 2021
f8d3b85
Use single thread executor
tomekzaw Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundEx
throw new RuntimeException("No blob module associated with BlobProvider");
}

byte[] data = blobModule.resolve(uri);
final byte[] data = blobModule.resolve(uri);
if (data == null) {
throw new FileNotFoundException("Cannot open " + uri.toString() + ", blob not found.");
}
Expand All @@ -84,13 +84,24 @@ public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundEx
return null;
}
ParcelFileDescriptor readSide = pipe[0];
ParcelFileDescriptor writeSide = pipe[1];

try (OutputStream outputStream = new ParcelFileDescriptor.AutoCloseOutputStream(writeSide)) {
outputStream.write(data);
} catch (IOException exception) {
return null;
}
final ParcelFileDescriptor writeSide = pipe[1];

tomekzaw marked this conversation as resolved.
Show resolved Hide resolved
Thread writer =
new Thread() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One pretty valid concern raised by @ShikaSD is that creating a new thread for each URI is a good idea can consume quite a bit of memory by accident.

What do you think about going ahead with your suggestion about conditionally creating he thread when the blob is bigger than the pipe?

Additionally, @ShikaSD suggested using an executor here to schedule things on a single thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about going ahead with your suggestion about conditionally creating he thread when the blob is bigger than the pipe?

I tried to determine the pipe capacity using Os.fcntlInt, but this method is only available from API 30 (see the documentation).

int F_GETPIPE_SZ = 1032; // Linux 2.6.35+
FileDescriptor fd = writeSide.getFileDescriptor();
int pipeCapacity = Os.fcntlInt(fd, F_GETPIPE_SZ, 0);

Probably we could call fcntl(2) from native code instead. Another way to determine the pipe capacity would be to open and read /proc/sys/fs/pipe-max-size. Hopefully it's enough to assign 65536 directly for now.

I've also updated the example in RNTester so now it includes two images – one smaller and one larger than 64 KB.

Updated RNTester

Just to be safe, I've also checked if it works properly for an image exactly of size equal to pipe capacity. In order to generate images of arbitrary size, I've implemented a HTTP server in Flask which appends null bytes to an existing JPG image and returns the modified image in the response:

import urllib

from flask import Flask, request, make_response

app = Flask(__name__)

url = 'https://upload.wikimedia.org/wikipedia/commons/thumb/9/97/The_Earth_seen_from_Apollo_17.jpg/240px-The_Earth_seen_from_Apollo_17.jpg'
original = urllib.request.urlopen(url).read()

@app.route("/image.jpg")
def image():
    size = int(request.args['size'])
    diff = size - len(original)
    assert diff >= 0

    output = original + b'\0' * diff
    assert len(output) == size

    response = make_response(output)
    response.headers.set('Content-Type', 'image/jpeg')
    return response

In RNTester app I've replaced the URLs:

<BlobImageExample
  urls={[
    'http://10.0.2.2:5000/image.jpg?size=65534',
    'http://10.0.2.2:5000/image.jpg?size=65535',
    'http://10.0.2.2:5000/image.jpg?size=65536', // max pipe capacity
    'http://10.0.2.2:5000/image.jpg?size=65537',
    'http://10.0.2.2:5000/image.jpg?size=65538',
  ]}
/>
Before (always synchronous write) After (conditional write)
Synchronous write Conditional write

Using the new implementation with conditional write, all images are loaded properly, so it should be safe to use the condition data.length <= PIPE_CAPACITY.

Additionally, @ShikaSD suggested using an executor here to schedule things on a single thread.

Good idea! Done.

public void run() {
try (OutputStream outputStream =
new ParcelFileDescriptor.AutoCloseOutputStream(writeSide)) {
// If the blob data is larger than pipe capacity (64 KB), a synchronous write call
// would fill up the whole buffer and block until the bytes are read (i.e. never).
// Writing from a separate thread allows us to return the read side descriptor
// immediately so that the reader can start reading.
outputStream.write(data);
} catch (IOException exception) {
// no-op
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens in this situation? Should we somehow close or notify with failure?

Previously, the parent method would return null instead of readSide.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the new implementation returns a ParcelFileDescriptor immediately, i.e. before calling write (which possibly could fail later on), we cannot return null as previously.

AutoCloseOutputStream ensures that the write side descriptor always gets closed, no matter if the write was successful or not. In case of an I/O error, the write side descriptor gets closed, and reading from a broken pipe will also result in an IOException on the reader side.

According to Android documentation, the reader is responsible for closing the read side descriptor:

The returned ParcelFileDescriptor is owned by the caller, so it is their responsibility to close it when done.

So it should be safe to return ParcelFileDescriptor instead of null, since the reader is responsible for closing it.

The opposite case is when the reader closes the read side descriptor on purpose, for example when unmounting an image component before it was fully loaded (i.e. partial read). In such case, the write call fails and the write side descriptor gets closed as well, so there is no resource leak.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! Thank you for the detailed explanation.

tomekzaw marked this conversation as resolved.
Show resolved Hide resolved
}
}
};
writer.start();

return readSide;
}
Expand Down
5 changes: 5 additions & 0 deletions packages/rn-tester/android/app/src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@
</intent-filter>
</activity>
<activity android:name="com.facebook.react.devsupport.DevSettingsActivity" />
<provider
android:name="com.facebook.react.modules.blob.BlobProvider"
android:authorities="@string/blob_provider_authority"
android:exported="false"
/>
</application>

</manifest>
1 change: 1 addition & 0 deletions packages/rn-tester/android/app/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<resources>
<string name="app_name">RNTester App</string>
<string name="blob_provider_authority">com.facebook.react.uiapp.blobs</string>
</resources>
44 changes: 44 additions & 0 deletions packages/rn-tester/js/examples/Image/ImageExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,40 @@ type ImageSource = $ReadOnly<{|
uri: string,
|}>;

type BlobImageExampleState = {|
objectURL: ?string,
|};

type BlobImageExampleProps = $ReadOnly<{|
url: string,
|}>;

class BlobImageExample extends React.Component<
BlobImageExampleProps,
BlobImageExampleState,
> {
state = {
objectURL: null,
};

UNSAFE_componentWillMount() {
(async () => {
const result = await fetch(this.props.url);
const blob = await result.blob();
const objectURL = URL.createObjectURL(blob);
this.setState({objectURL});
})();
}

render() {
return this.state.objectURL !== null ? (
<Image source={{uri: this.state.objectURL}} style={styles.base} />
) : (
<Text>Object URL not created yet</Text>
);
}
}

type NetworkImageCallbackExampleState = {|
events: Array<string>,
startLoadPrefetched: boolean,
Expand Down Expand Up @@ -608,6 +642,16 @@ exports.examples = [
return <Image source={fullImage} style={styles.base} />;
},
},
{
title: 'Plain Blob Image',
description: ('If the `source` prop `uri` property is an object URL, ' +
'then it will be resolved using `BlobProvider` (Android) or `RCTBlobManager` (iOS).': string),
render: function(): React.Node {
return (
<BlobImageExample url="https://www.facebook.com/ads/pics/successstories.png" />
);
},
},
{
title: 'Plain Static Image',
description: ('Static assets should be placed in the source code tree, and ' +
Expand Down