Skip to content

Commit 44b1914

Browse files
rommansabbirrommansabbir
authored
fix: exception on concurrent download of ParseFile from multiple threads (#1180)
1 parent 351858c commit 44b1914

File tree

2 files changed

+141
-58
lines changed

2 files changed

+141
-58
lines changed

‎parse/src/main/java/com/parse/ParseFileController.java

+77-58
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
import com.parse.http.ParseHttpRequest;
1313
import java.io.File;
1414
import java.io.IOException;
15+
import java.util.ArrayList;
16+
import java.util.List;
1517
import java.util.concurrent.CancellationException;
1618
import org.json.JSONObject;
1719

@@ -21,6 +23,7 @@ class ParseFileController {
2123
private final Object lock = new Object();
2224
private final ParseHttpClient restClient;
2325
private final File cachePath;
26+
private final List<String> currentlyDownloadedFilesNames = new ArrayList<>();
2427

2528
private ParseHttpClient fileClient;
2629

@@ -168,64 +171,80 @@ public Task<File> fetchAsync(
168171
if (cancellationToken != null && cancellationToken.isCancelled()) {
169172
return Task.cancelled();
170173
}
171-
final File cacheFile = getCacheFile(state);
172-
return Task.call(cacheFile::exists, ParseExecutors.io())
173-
.continueWithTask(
174-
task -> {
175-
boolean result = task.getResult();
176-
if (result) {
177-
return Task.forResult(cacheFile);
178-
}
179-
if (cancellationToken != null && cancellationToken.isCancelled()) {
180-
return Task.cancelled();
174+
return Task.call(
175+
() -> {
176+
final File cacheFile = getCacheFile(state);
177+
178+
synchronized (lock) {
179+
if (currentlyDownloadedFilesNames.contains(state.name())) {
180+
while (currentlyDownloadedFilesNames.contains(state.name())) {
181+
lock.wait();
181182
}
182-
183-
// Generate the temp file path for caching ParseFile content based on
184-
// ParseFile's url
185-
// The reason we do not write to the cacheFile directly is because there
186-
// is no way we can
187-
// verify if a cacheFile is complete or not. If download is interrupted
188-
// in the middle, next
189-
// time when we download the ParseFile, since cacheFile has already
190-
// existed, we will return
191-
// this incomplete cacheFile
192-
final File tempFile = getTempFile(state);
193-
194-
// network
195-
final ParseFileRequest request =
196-
new ParseFileRequest(
197-
ParseHttpRequest.Method.GET, state.url(), tempFile);
198-
199-
// We do not need to delete the temp file since we always try to
200-
// overwrite it
201-
return request.executeAsync(
202-
fileClient(),
203-
null,
204-
downloadProgressCallback,
205-
cancellationToken)
206-
.continueWithTask(
207-
task1 -> {
208-
// If the top-level task was cancelled, don't
209-
// actually set the data -- just move on.
210-
if (cancellationToken != null
211-
&& cancellationToken.isCancelled()) {
212-
throw new CancellationException();
213-
}
214-
if (task1.isFaulted()) {
215-
ParseFileUtils.deleteQuietly(tempFile);
216-
return task1.cast();
217-
}
218-
219-
// Since we give the cacheFile pointer to
220-
// developers, it is not safe to guarantee
221-
// cacheFile always does not exist here, so it is
222-
// better to delete it manually,
223-
// otherwise moveFile may throw an exception.
224-
ParseFileUtils.deleteQuietly(cacheFile);
225-
ParseFileUtils.moveFile(tempFile, cacheFile);
226-
return Task.forResult(cacheFile);
227-
},
228-
ParseExecutors.io());
229-
});
183+
}
184+
185+
if (cacheFile.exists()) {
186+
return cacheFile;
187+
} else {
188+
currentlyDownloadedFilesNames.add(state.name());
189+
}
190+
}
191+
192+
try {
193+
if (cancellationToken != null && cancellationToken.isCancelled()) {
194+
throw new CancellationException();
195+
}
196+
197+
// Generate the temp file path for caching ParseFile content based on
198+
// ParseFile's url
199+
// The reason we do not write to the cacheFile directly is because there
200+
// is no way we can
201+
// verify if a cacheFile is complete or not. If download is interrupted
202+
// in the middle, next
203+
// time when we download the ParseFile, since cacheFile has already
204+
// existed, we will return
205+
// this incomplete cacheFile
206+
final File tempFile = getTempFile(state);
207+
208+
// network
209+
final ParseFileRequest request =
210+
new ParseFileRequest(
211+
ParseHttpRequest.Method.GET, state.url(), tempFile);
212+
213+
// We do not need to delete the temp file since we always try to
214+
// overwrite it
215+
Task<Void> downloadTask =
216+
request.executeAsync(
217+
fileClient(),
218+
null,
219+
downloadProgressCallback,
220+
cancellationToken);
221+
ParseTaskUtils.wait(downloadTask);
222+
223+
// If the top-level task was cancelled, don't
224+
// actually set the data -- just move on.
225+
if (cancellationToken != null && cancellationToken.isCancelled()) {
226+
throw new CancellationException();
227+
}
228+
if (downloadTask.isFaulted()) {
229+
ParseFileUtils.deleteQuietly(tempFile);
230+
throw new RuntimeException(downloadTask.getError());
231+
}
232+
233+
// Since we give the cacheFile pointer to
234+
// developers, it is not safe to guarantee
235+
// cacheFile always does not exist here, so it is
236+
// better to delete it manually,
237+
// otherwise moveFile may throw an exception.
238+
ParseFileUtils.deleteQuietly(cacheFile);
239+
ParseFileUtils.moveFile(tempFile, cacheFile);
240+
return cacheFile;
241+
} finally {
242+
synchronized (lock) {
243+
currentlyDownloadedFilesNames.remove(state.name());
244+
lock.notifyAll();
245+
}
246+
}
247+
},
248+
ParseExecutors.io());
230249
}
231250
}

‎parse/src/test/java/com/parse/ParseFileControllerTest.java

+64
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
import java.io.IOException;
2929
import java.net.MalformedURLException;
3030
import java.net.URL;
31+
import java.util.concurrent.CountDownLatch;
32+
import java.util.concurrent.atomic.AtomicReference;
3133
import org.json.JSONObject;
3234
import org.junit.After;
3335
import org.junit.Before;
@@ -341,5 +343,67 @@ public void testFetchAsyncFailure() throws Exception {
341343
assertFalse(controller.getTempFile(state).exists());
342344
}
343345

346+
@Test
347+
public void testFetchAsyncConcurrentCallsSuccess() throws Exception {
348+
byte[] data = "hello".getBytes();
349+
ParseHttpResponse mockResponse =
350+
new ParseHttpResponse.Builder()
351+
.setStatusCode(200)
352+
.setTotalSize((long) data.length)
353+
.setContent(new ByteArrayInputStream(data))
354+
.build();
355+
356+
ParseHttpClient fileClient = mock(ParseHttpClient.class);
357+
when(fileClient.execute(any(ParseHttpRequest.class))).thenReturn(mockResponse);
358+
// Make sure cache dir does not exist
359+
File root = new File(temporaryFolder.getRoot(), "cache");
360+
assertFalse(root.exists());
361+
ParseFileController controller = new ParseFileController(null, root).fileClient(fileClient);
362+
ParseFile.State state = new ParseFile.State.Builder().name("file_name").url("url").build();
363+
CountDownLatch countDownLatch = new CountDownLatch(2);
364+
AtomicReference<File> file1Ref = new AtomicReference<>();
365+
AtomicReference<File> file2Ref = new AtomicReference<>();
366+
367+
new Thread(
368+
() -> {
369+
try {
370+
file1Ref.set(
371+
ParseTaskUtils.wait(
372+
controller.fetchAsync(state, null, null, null)));
373+
} catch (ParseException e) {
374+
throw new RuntimeException(e);
375+
} finally {
376+
countDownLatch.countDown();
377+
}
378+
})
379+
.start();
380+
381+
new Thread(
382+
() -> {
383+
try {
384+
file2Ref.set(
385+
ParseTaskUtils.wait(
386+
controller.fetchAsync(state, null, null, null)));
387+
} catch (ParseException e) {
388+
throw new RuntimeException(e);
389+
} finally {
390+
countDownLatch.countDown();
391+
}
392+
})
393+
.start();
394+
395+
countDownLatch.await();
396+
397+
File result1 = file1Ref.get();
398+
File result2 = file2Ref.get();
399+
400+
assertTrue(result1.exists());
401+
assertEquals("hello", ParseFileUtils.readFileToString(result1, "UTF-8"));
402+
assertTrue(result2.exists());
403+
assertEquals("hello", ParseFileUtils.readFileToString(result2, "UTF-8"));
404+
verify(fileClient, times(1)).execute(any(ParseHttpRequest.class));
405+
assertFalse(controller.getTempFile(state).exists());
406+
}
407+
344408
// endregion
345409
}

0 commit comments

Comments
 (0)