-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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.
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
Fix support for blobs larger than 64 KB on Android #31789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
google-java-format
found some issues. See https://github.com/google/google-java-format
ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobProvider.java
Show resolved
Hide resolved
Base commit: fa0518d |
Base commit: fa0518d |
We can also optimize the new implementation to start a new thread only if the blob is larger than 64 KB, otherwise write to pipe synchronously. |
// immediately so that the reader can start reading. | ||
outputStream.write(data); | ||
} catch (IOException exception) { | ||
// no-op |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
final ParcelFileDescriptor writeSide = pipe[1]; | ||
|
||
Thread writer = | ||
new Thread() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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) |
---|---|
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
google-java-format
found some issues. See https://github.com/google/google-java-format
ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobProvider.java
Outdated
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobProvider.java
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobProvider.java
Outdated
Show resolved
Hide resolved
@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@yungsters merged this pull request in f00e348. |
Summary: Fixes #31774. This pull request resolves a problem related to accessing blobs greater than 64 KB on Android. When an object URL for such blob is passed as source of `<Image />` component, the image does not load. This issue was related to the fact that pipe buffer has a limited capacity of 65536 bytes (https://man7.org/linux/man-pages/man7/pipe.7.html, section "Pipe capacity"). If there is more bytes to be written than free space in the buffer left, the write operation blocks and waits until the content is read from the pipe. The current implementation of `BlobProvider.openFile` first creates a pipe, then writes the blob data to the pipe and finally returns the read side descriptor of the pipe. For blobs larger than 64 KB, the write operation will block forever, because there are no readers to empty the buffer. https://github.com/facebook/react-native/blob/41ecccefcf16ac8bcf858dd955af709eb20f7e4a/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobProvider.java#L86-L95 This pull request moves the write operation to a separate thread. The read side descriptor is returned immediately so that both writer and reader can work simultaneously. Reading from the pipe empties the buffer and allows the next chunks to be written. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Android] [Fixed] - Fix support for blobs larger than 64 KB Pull Request resolved: #31789 Test Plan: A new example has been added to RN Tester app to verify if the new implementation properly loads the image of size 455 KB from a blob via object URL passed as image source. <img src="https://user-images.githubusercontent.com/20516055/123859163-9eba6d80-d924-11eb-8a09-2b1f353bb968.png" alt="Screenshot_1624996413" width="300" /> Reviewed By: ShikaSD Differential Revision: D29674273 Pulled By: yungsters fbshipit-source-id: e0ac3ec0a23690b05ab843061803f95f7666c0db
Summary
Fixes #31774.
This pull request resolves a problem related to accessing blobs greater than 64 KB on Android. When an object URL for such blob is passed as source of
<Image />
component, the image does not load.This issue was related to the fact that pipe buffer has a limited capacity of 65536 bytes (https://man7.org/linux/man-pages/man7/pipe.7.html, section "Pipe capacity"). If there is more bytes to be written than free space in the buffer left, the write operation blocks and waits until the content is read from the pipe.
The current implementation of
BlobProvider.openFile
first creates a pipe, then writes the blob data to the pipe and finally returns the read side descriptor of the pipe. For blobs larger than 64 KB, the write operation will block forever, because there are no readers to empty the buffer.react-native/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobProvider.java
Lines 86 to 95 in 41eccce
This pull request moves the write operation to a separate thread. The read side descriptor is returned immediately so that both writer and reader can work simultaneously. Reading from the pipe empties the buffer and allows the next chunks to be written.
Changelog
[Android] [Fixed] - Fix support for blobs larger than 64 KB
Test Plan
A new example has been added to RN Tester app to verify if the new implementation properly loads the image of size 455 KB from a blob via object URL passed as image source.