stopDownload does not cause download promise to reject

[Alatius]

The documentation says about stopDownload that "the promise returned from the aborted downloadFile() call will reject with an error." This works fine on Android, but not on iOS: the download stops, but the promise remains. This seems to be an old issue, and I found this pull request to another fork: https://github.com/itinance/react-native-fs/pull/824 I have tried applying it locally to test it, and it seems to solve the problem, as far as I can see.

[Alatius]

Having thought some more on this, maybe the current behaviour is by design?

If one does not have any intention of supporting resume, one could then supposedly do something like this:

const handleCancel = () => {
  // Clean up after cancellation
};

const handleFinish = () => {
  // What you want to do after both successful and cancelled download
};

try {
  const { jobId, promise } = downloadFile({
    fromUrl,
    toFile,
    resumable: (res) => {
      handleCancel();
      handleFinish();
    },
  });

  const result = await promise;

  // ...

} catch (error) {
  handleCancel();
} finally {
  handleFinish();
}

This works pretty well, but the promise will remain unresolved and unrejected, which feels like a memory leak.

Maybe a good compromise is to keep the current behaviour if resumable is set, and if it is not set, reject the promise when the download is stopped.

[birdofpreyru]

Hi @Alatius , thanks for digging into it!

On your first message I though — great, let's adopt the PR from the upstream, an easy win!

On your second message, it seems, it deserves some thinking. My gut feeling tells — maybe reject the initial promise on stopDownload(), and modify resumeDownload() to return a new promise for the resumed download. Though, I'd look what NodeJS / other libraries do in similar scenarios, and copy the «mainstream» behavior.

I don't like your idea with different behavior dependent on resume value, because it feels very probable that someone makes use of stopDownload() without resume, wires some functionality to it, and then if he attempts to add «resume» feature down the line he'll probably bump into breaking his existing logic if the stopping feature behaves different when resume is enabled.

[Alatius]

Yeah, it is a fair point that introducing changes in the behaviour risks breaking the implementation for some people, and maybe my suggestion is not the most elegant. I guess that the reverse situation to your scenario is also a possibility: that someone has implemented a resume functionality without making use of resumable. Then my proposal would break that code, as the promise will not be there anymore to handle the resume.

But if we want to minimize the risk of introducing breaking changes, I fear that the solution you suggest may equally, or even more, risk breaking current code, wouldn't it?

It seems to me that the root of the problem is that a method to actually fully cancel a download is strangely missing: the intention behind stopDownload() seems to be primarily to pause the download, until it can be resumed, and the original promise can finish. The situation where resume is not available (i.e. on Android) should then be seen as an exception, and is indeed treated as an error.

So maybe the proper solution is to introduce a new function, cancelDownload(), which always rejects the promise? (Or, alternatively, always resolves it with some informative message?) That way, nothing risks breaking, as no changes to the behaviour of current implementations are introduced. I have not looked more into other similar libraries however; as you say, it is probably a good idea to have similar logic to what people are accustomed to.

[birdofpreyru]

I mean, I have no problem with a breaking change between major/minor library versions, I was talking about changing the value of resume parameter while using the very same version of the library — if it will change the resolution/rejection behavior of stopDownload() promise, it would feel very unexpected and annoying to me.

[Alatius]

Aha, yes, I think I understand your point now. But I wouldn't say that it changes stopDownload() per se. Rather, the way I look at it is that it would change the behaviour of the promise returned from downloadFile(): if resumable (note, not "resume") is not set (or "false") when calling downloadFile(), the download promise will not be "resumable", even on iOS, but will instead reject when stopDownload() is called. If resumable is set to a callback function (thus logically "true"), the download will be resumable (on the platform that supports it). This seems pretty logical, and what would be expected to me.

I am still not sure what solution is best though: all proposals mentioned (except for the old pull request I linked to in my first message) have their merits I think. To bring it in line with other similar libraries is probably the way to go, instead of reinventing wheels.