Skip to content

Commit 1574e27

Browse files
fivecarfivecar
authored
fix: getAvailableUrl returns remote when lazy-deleted (#36)
Also made the way we revive some tasks more robust
1 parent 8a355de commit 1574e27

File tree

2 files changed

+147
-19
lines changed

2 files changed

+147
-19
lines changed

‎src/index.ts‎

Lines changed: 55 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -271,19 +271,7 @@ export default class DownloadQueue {
271271
if (existingTasks.some(task => task.id === spec.id)) {
272272
return;
273273
}
274-
if (spec.finished) {
275-
// Once in a while we think the spec is finished but the file isn't
276-
// on disk. This can happen when XCode installs a new build, and
277-
// sometimes through TestFlight.
278-
const exists = await RNFS.exists(spec.path);
279-
280-
if (exists) {
281-
return;
282-
}
283-
284-
spec.finished = false; // We're not really finished, it seems.
285-
await this.kvfs.write(this.keyFromId(spec.id), spec);
286-
}
274+
await this.reconcileFinishStateWithFile(spec);
287275
return this.start(spec);
288276
})
289277
);
@@ -613,7 +601,8 @@ export default class DownloadQueue {
613601

614602
/**
615603
* Gets a remote or local url, preferring to the local path when possible. If
616-
* the local file hasn't yet been downloaded, returns the remote url.
604+
* the local file hasn't yet been downloaded, returns the remote url. Also
605+
* returns the remote url if the record is being lazy-deleted.
617606
* @param url The remote URL to check for local availability
618607
* @returns A local file path if the URL has already been downloaded, else url
619608
*/
@@ -622,7 +611,7 @@ export default class DownloadQueue {
622611

623612
const spec = this.specs.find(spec => spec.url === url);
624613

625-
if (!spec || !spec.finished) {
614+
if (!spec || !spec.finished || spec.createTime <= 0) {
626615
return url;
627616
}
628617

@@ -812,6 +801,10 @@ export default class DownloadQueue {
812801
private async reviveTask(task: DownloadTask) {
813802
const spec = this.specs.find(spec => spec.id === task.id);
814803

804+
if (spec) {
805+
await this.reconcileFinishStateWithFile(spec);
806+
}
807+
815808
// Don't revive finished tasks or ones that already have lazy deletes in
816809
// progress.
817810
if (spec && !spec.finished && spec.createTime > 0) {
@@ -827,9 +820,27 @@ export default class DownloadQueue {
827820
this.handlers?.onBegin?.(spec.url, task.totalBytes);
828821
break;
829822
case "DONE":
830-
this.handlers?.onBegin?.(spec.url, task.totalBytes);
831-
this.handlers?.onDone?.(spec.url, spec.path);
832-
shouldAddTask = false;
823+
{
824+
const exists = await RNFS.exists(spec.path);
825+
826+
if (exists) {
827+
spec.finished = true;
828+
await this.kvfs.write(this.keyFromId(spec.id), spec);
829+
this.handlers?.onBegin?.(spec.url, task.totalBytes);
830+
this.handlers?.onDone?.(spec.url, spec.path);
831+
shouldAddTask = false;
832+
} else {
833+
// If the spec thinks we're not done but the OS does, yet we
834+
// can't find the file on disk, we'll leave shouldAddTask = true so
835+
// that we begin the download again.
836+
// Downloader docs say every task needs to be paused or stopped.
837+
// So we stop here.
838+
task.stop();
839+
// Since the file is missing from disk, yet the downloader thinks
840+
// it's done, we restart the download.
841+
this.start(spec);
842+
}
843+
}
833844
break;
834845
case "STOPPED":
835846
this.start(spec);
@@ -858,7 +869,7 @@ export default class DownloadQueue {
858869
task.stop();
859870
}
860871
} else {
861-
if (["DOWNLOADING", "PAUSED"].includes(task.state)) {
872+
if (this.isTaskDownloading(task)) {
862873
task.stop();
863874

864875
if (spec && !spec.finished) {
@@ -876,6 +887,31 @@ export default class DownloadQueue {
876887
}
877888
}
878889

890+
/**
891+
* Makes sure, if a spec thinks it's finished, that the file which backs it
892+
* actually exists. If that file doesn't exist, we set finished === false.
893+
* @returns true if spec is finished and the file exists
894+
*/
895+
private async reconcileFinishStateWithFile(spec: Spec) {
896+
if (spec.finished) {
897+
// Once in a while we think the spec is finished but the file isn't
898+
// on disk. This can happen when XCode installs a new build, and
899+
// sometimes through TestFlight.
900+
const exists = await RNFS.exists(spec.path);
901+
902+
if (exists) {
903+
return;
904+
}
905+
906+
spec.finished = false; // We're not really finished, it seems.
907+
await this.kvfs.write(this.keyFromId(spec.id), spec);
908+
}
909+
}
910+
911+
private isTaskDownloading(task: DownloadTask) {
912+
return ["DOWNLOADING", "PAUSED"].includes(task.state);
913+
}
914+
879915
private extensionFromUri(uri: string) {
880916
const path = this.urlToPath?.(uri);
881917

‎test/index.spec.ts‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,7 @@ describe("DownloadQueue", () => {
383383
task.state = "DONE";
384384
task.totalBytes = 8675309;
385385
(checkForExistingDownloads as jest.Mock).mockReturnValue([task]);
386+
(exists as jest.Mock).mockReturnValue(true);
386387

387388
await kvfs.write("/mydomain/foo", {
388389
id: "foo",
@@ -405,6 +406,33 @@ describe("DownloadQueue", () => {
405406
expect(download).not.toHaveBeenCalled();
406407
});
407408

409+
it("revives done specs from previous launches with missing files", async () => {
410+
const queue = new DownloadQueue();
411+
const handlers: DownloadQueueHandlers = {
412+
onBegin: jest.fn(),
413+
onDone: jest.fn(),
414+
};
415+
416+
task.state = "DONE";
417+
task.totalBytes = 8675309;
418+
(checkForExistingDownloads as jest.Mock).mockReturnValue([task]);
419+
(exists as jest.Mock).mockReturnValue(false);
420+
421+
await kvfs.write("/mydomain/foo", {
422+
id: "foo",
423+
url: "http://foo.com/a.mp3",
424+
path: `${RNFS.DocumentDirectoryPath}/DownloadQueue/mydomain/foo`,
425+
createTime: Date.now() - 1000,
426+
});
427+
await queue.init({ domain: "mydomain", handlers });
428+
429+
// Because it's done downloading, we don't expect resume()
430+
expect(task.resume).not.toHaveBeenCalled();
431+
expect(handlers.onBegin).not.toHaveBeenCalled();
432+
expect(handlers.onDone).not.toHaveBeenCalled();
433+
expect(download).toHaveBeenCalledTimes(1);
434+
});
435+
408436
it("restarts stopped specs from previous launches", async () => {
409437
const queue = new DownloadQueue();
410438
const handlers: DownloadQueueHandlers = {
@@ -1942,6 +1970,70 @@ describe("DownloadQueue", () => {
19421970
);
19431971
});
19441972

1973+
it("should give you back the remote url when spec is lazy-deleted", async () => {
1974+
const queue = new DownloadQueue();
1975+
const fooTask = createBasicTask();
1976+
let fooPath = "tbd";
1977+
1978+
(download as jest.Mock).mockImplementation(
1979+
(spec: { id: string; url: string; destination: string }) => {
1980+
if (spec.url === "http://foo.com/a.mp3") {
1981+
fooPath = spec.destination;
1982+
return Object.assign(fooTask, {
1983+
done: jest.fn((handler: DoneHandler) => {
1984+
fooTask._done = handler;
1985+
return fooTask;
1986+
}),
1987+
});
1988+
}
1989+
return {
1990+
...task,
1991+
id: spec.id,
1992+
path: spec.destination,
1993+
};
1994+
}
1995+
);
1996+
1997+
await queue.init({ domain: "mydomain" });
1998+
await queue.addUrl("http://foo.com/a.mp3");
1999+
await queue.addUrl("http://boo.com/a.mp3");
2000+
2001+
// Pretend we've downloaded only foo
2002+
(exists as jest.Mock).mockImplementation(path => path === fooPath);
2003+
2004+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
2005+
await fooTask._done!();
2006+
await queue.removeUrl("http://foo.com/a.mp3", Date.now() + 50000);
2007+
2008+
const [fooU, booU] = await Promise.all([
2009+
queue.getAvailableUrl("http://foo.com/a.mp3"),
2010+
queue.getAvailableUrl("http://boo.com/a.mp3"),
2011+
]);
2012+
2013+
expect(fooU).toBe("http://foo.com/a.mp3"); // Should give us remote URL
2014+
expect(booU).toBe("http://boo.com/a.mp3");
2015+
2016+
const restartedQueue = new DownloadQueue();
2017+
2018+
await restartedQueue.init({ domain: "mydomain" });
2019+
const [fooUR, statuses] = await Promise.all([
2020+
restartedQueue.getAvailableUrl("http://foo.com/a.mp3"),
2021+
restartedQueue.getQueueStatus(),
2022+
]);
2023+
2024+
// We should be sure that the lazy-deleted status is reported as !complete
2025+
expect(fooUR).toBe("http://foo.com/a.mp3"); // Should give us remote URL
2026+
expect(statuses.length).toBe(1); // only boo should be left
2027+
expect(statuses).toEqual(
2028+
expect.arrayContaining([
2029+
expect.objectContaining({
2030+
url: "http://boo.com/a.mp3",
2031+
complete: false,
2032+
}),
2033+
])
2034+
);
2035+
});
2036+
19452037
it("should call handlers for all cases", async () => {
19462038
const handlers: DownloadQueueHandlers = {
19472039
onBegin: jest.fn(),

0 commit comments

Comments
 (0)