Skip to content

Download ParseFile content to file instead of memory #53

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
59 changes: 34 additions & 25 deletions Parse/src/main/java/com/parse/ParseAWSRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,29 @@
*/
package com.parse;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.File;
import java.io.FileOutputStream;
import java.io.InputStream;
import java.util.concurrent.Callable;

import bolts.Task;

/**
* Request returns a byte array of the response and provides a callback the progress of the data
* read from the network.
*/
/** package */ class ParseAWSRequest extends ParseRequest<byte[]> {
/** package */ class ParseAWSRequest extends ParseRequest<Void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be easier if we made the generic File?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, what ParseAWSRequest do is to read response stream and write data to a tempFile. Since we have already injected the tempFile (which means the caller already holds the file pointer), I change byte[] to Void. But if you think return the tempFile is cleaner, I can change this.


public ParseAWSRequest(ParseHttpRequest.Method method, String url) {
// The temp file is used to save the ParseFile content when we fetch it from server
private final File tempFile;

public ParseAWSRequest(ParseHttpRequest.Method method, String url, File tempFile) {
super(method, url);
this.tempFile = tempFile;
}

@Override
protected Task<byte[]> onResponseAsync(ParseHttpResponse response,
protected Task<Void> onResponseAsync(final ParseHttpResponse response,
final ProgressCallback downloadProgressCallback) {
int statusCode = response.getStatusCode();
if (statusCode >= 200 && statusCode < 300 || statusCode == 304) {
Expand All @@ -40,29 +45,33 @@ protected Task<byte[]> onResponseAsync(ParseHttpResponse response,
return null;
}

long totalSize = response.getTotalSize();
int downloadedSize = 0;
InputStream responseStream = null;
try {
responseStream = response.getContent();
ByteArrayOutputStream buffer = new ByteArrayOutputStream();
return Task.call(new Callable<Void>() {
@Override
public Void call() throws Exception {
long totalSize = response.getTotalSize();
long downloadedSize = 0;
InputStream responseStream = null;
try {
responseStream = response.getContent();
FileOutputStream tempFileStream = new FileOutputStream(tempFile);

int nRead;
byte[] data = new byte[32 << 10]; // 32KB
int nRead;
byte[] data = new byte[32 << 10]; // 32KB

while ((nRead = responseStream.read(data, 0, data.length)) != -1) {
buffer.write(data, 0, nRead);
downloadedSize += nRead;
if (downloadProgressCallback != null && totalSize != -1) {
int progressToReport = Math.round((float) downloadedSize / (float) totalSize * 100.0f);
downloadProgressCallback.done(progressToReport);
while ((nRead = responseStream.read(data, 0, data.length)) != -1) {
tempFileStream.write(data, 0, nRead);
downloadedSize += nRead;
if (downloadProgressCallback != null && totalSize != -1) {
int progressToReport =
Math.round((float) downloadedSize / (float) totalSize * 100.0f);
downloadProgressCallback.done(progressToReport);
}
}
return null;
} finally {
ParseIOUtils.closeQuietly(responseStream);
}
}
return Task.forResult(buffer.toByteArray());
} catch (IOException e) {
return Task.forError(e);
} finally {
ParseIOUtils.closeQuietly(responseStream);
}
}, ParseExecutors.io());
}
}
44 changes: 30 additions & 14 deletions Parse/src/main/java/com/parse/ParseFileController.java
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ public File getCacheFile(ParseFile.State state) {
return new File(cachePath, state.name());
}

/* package for tests */ File getTempFile(ParseFile.State state) {
if (state.url() == null) {
return null;
}
return new File(cachePath, state.url() + ".tmp");
}

public boolean isDataAvailable(ParseFile.State state) {
return getCacheFile(state).exists();
}
Expand Down Expand Up @@ -172,46 +179,55 @@ public Task<File> fetchAsync(
if (cancellationToken != null && cancellationToken.isCancelled()) {
return Task.cancelled();
}
final File file = getCacheFile(state);
final File cacheFile = getCacheFile(state);
return Task.call(new Callable<Boolean>() {
@Override
public Boolean call() throws Exception {
return file.exists();
return cacheFile.exists();
}
}, Task.BACKGROUND_EXECUTOR).continueWithTask(new Continuation<Boolean, Task<File>>() {
}, ParseExecutors.io()).continueWithTask(new Continuation<Boolean, Task<File>>() {
@Override
public Task<File> then(Task<Boolean> task) throws Exception {
boolean result = task.getResult();
if (result) {
return Task.forResult(file);
return Task.forResult(cacheFile);
}
if (cancellationToken != null && cancellationToken.isCancelled()) {
return Task.cancelled();
}

// Generate the temp file path for caching ParseFile content based on ParseFile's url
// The reason we do not write to the cacheFile directly is because there is no way we can
// verify if a cacheFile is complete or not. If download is interrupted in the middle, next
// time when we download the ParseFile, since cacheFile has already existed, we will return
// this incomplete cacheFile
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense since although we use a TaskQueue in ParseFile, there could potentially be multiple instances of the same ParseFile without LDS.

Would it make sense to instead keep a mapping of URL -> Task<File> so that we return the Task<File> if the same URL is attempted to be fetched multiple times? This would reduce the amount of requests as well as remove the potential error when we update the file pointer when a developer is reading it due to concurrent fetches on the same ParseFile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is useful but we have already had cacheFile. Before we fetch, we check whether the cacheFile exist or not. So instead of checking file exists or not in disk, you want to keep a in-memory map to make the checking faster?

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, I thought this comment was with regards to asynchronously downloading a ParseFile, but it's not.

final File tempFile = getTempFile(state);

// network
final ParseAWSRequest request = new ParseAWSRequest(ParseHttpRequest.Method.GET, state.url());
final ParseAWSRequest request =
new ParseAWSRequest(ParseHttpRequest.Method.GET, state.url(), tempFile);

// TODO(grantland): Stream response directly to file t5042019
// We do not need to delete the temp file since we always try to overwrite it
return request.executeAsync(
awsClient(),
null,
downloadProgressCallback,
cancellationToken).onSuccess(new Continuation<byte[], File>() {
cancellationToken).continueWithTask(new Continuation<Void, Task<File>>() {
@Override
public File then(Task<byte[]> task) throws Exception {
public Task<File> then(Task<Void> task) throws Exception {
// If the top-level task was cancelled, don't actually set the data -- just move on.
if (cancellationToken != null && cancellationToken.isCancelled()) {
throw new CancellationException();
}

byte[] data = task.getResult();
if (data != null) {
ParseFileUtils.writeByteArrayToFile(file, data);
if (task.isFaulted()) {
ParseFileUtils.deleteQuietly(tempFile);
return task.cast();
}
return file;

ParseFileUtils.moveFile(tempFile, cacheFile);
return Task.forResult(cacheFile);
}
});
}, ParseExecutors.io());
}
});
}
Expand Down
7 changes: 5 additions & 2 deletions Parse/src/test/java/com/parse/ParseAWSRequestTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@

import org.junit.Rule;
import org.junit.rules.ExpectedException;
import org.junit.rules.TemporaryFolder;

import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.InputStream;

import bolts.Task;
Expand Down Expand Up @@ -44,8 +46,9 @@ public void test4XXThrowsException() throws Exception {
ParseHttpClient mockHttpClient = mock(ParseHttpClient.class);
when(mockHttpClient.execute(any(ParseHttpRequest.class))).thenReturn(mockResponse);

ParseAWSRequest request = new ParseAWSRequest(ParseHttpRequest.Method.GET, "http://parse.com");
Task<byte[]> task = request.executeAsync(mockHttpClient);
ParseAWSRequest request =
new ParseAWSRequest(ParseHttpRequest.Method.GET, "http://parse.com", null);
Task<Void> task = request.executeAsync(mockHttpClient);
task.waitForCompletion();

assertTrue(task.isFaulted());
Expand Down
5 changes: 5 additions & 0 deletions Parse/src/test/java/com/parse/ParseFileControllerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -302,13 +302,15 @@ public void testFetchAsyncSuccess() throws Exception {

ParseFile.State state = new ParseFile.State.Builder()
.name("file_name")
.url("url")
.build();
Task<File> task = controller.fetchAsync(state, null, null, null);
File result = ParseTaskUtils.wait(task);

verify(awsClient, times(1)).execute(any(ParseHttpRequest.class));
assertTrue(result.exists());
assertEquals("hello", ParseFileUtils.readFileToString(result, "UTF-8"));
assertFalse(controller.getTempFile(state).exists());
}

@Test
Expand All @@ -322,7 +324,9 @@ public void testFetchAsyncFailure() throws Exception {
File root = temporaryFolder.getRoot();
ParseFileController controller = new ParseFileController(null, root).awsClient(awsClient);

// We need to set url to make getTempFile() work and check it
ParseFile.State state = new ParseFile.State.Builder()
.url("test")
.build();
Task<File> task = controller.fetchAsync(state, null, null, null);
task.waitForCompletion();
Expand All @@ -334,6 +338,7 @@ public void testFetchAsyncFailure() throws Exception {
assertThat(error, instanceOf(ParseException.class));
assertEquals(ParseException.CONNECTION_FAILED, ((ParseException) error).getCode());
assertEquals(0, root.listFiles().length);
assertFalse(controller.getTempFile(state).exists());
}

//endregion
Expand Down
14 changes: 11 additions & 3 deletions Parse/src/test/java/com/parse/ParseRequestTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;

import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
import java.util.LinkedList;
import java.util.List;
Expand All @@ -34,6 +37,9 @@ public class ParseRequestTest {

private static byte[] data;

@Rule
public TemporaryFolder temporaryFolder = new TemporaryFolder();

@BeforeClass
public static void setUpClass() {
char[] chars = new char[64 << 10]; // 64KB
Expand Down Expand Up @@ -78,13 +84,15 @@ public void testDownloadProgress() throws Exception {
ParseHttpClient mockHttpClient = mock(ParseHttpClient.class);
when(mockHttpClient.execute(any(ParseHttpRequest.class))).thenReturn(mockResponse);

ParseAWSRequest request = new ParseAWSRequest(ParseHttpRequest.Method.GET, "localhost");
File tempFile = temporaryFolder.newFile("test");
ParseAWSRequest request =
new ParseAWSRequest(ParseHttpRequest.Method.GET, "localhost", tempFile);
TestProgressCallback downloadProgressCallback = new TestProgressCallback();
Task<byte[]> task = request.executeAsync(mockHttpClient, null, downloadProgressCallback);
Task<Void> task = request.executeAsync(mockHttpClient, null, downloadProgressCallback);

task.waitForCompletion();
assertFalse("Download failed: " + task.getError(), task.isFaulted());
assertEquals(data.length, task.getResult().length);
assertEquals(data.length, ParseFileUtils.readFileToByteArray(tempFile).length);

assertProgressCompletedSuccessfully(downloadProgressCallback);
}
Expand Down