novoda / download-manager

A library that handles long-running downloads, handling the network interactions and retrying downloads automatically after failures
Apache License 2.0
483 stars 63 forks source link

DownloadBatchStatusCallback triggers for downloaded items after WAITING_FOR_NETWORK #473

Open mspmax opened 5 years ago

mspmax commented 5 years ago

Guys,

As always I appreciate your work :). This is a question rather than a bug but because of this, it's causing an issue in my implementation.

Basically, I noticed that when the system goes offline every(in progress/queued etc ) batch goes to WAITING_FOR_NETWORK state. Which makes sense. But when we go back online I noticed the DownloadBatchStatusCallback will trigger onUpdate even for already downloaded batches. Is there any reason for this? or is it a bug?

Thanks in advance!

Mecharyry commented 5 years ago

Hi @mspmax, as far as I know this is the correct behaviour. Inside the network recovery we perform a LiteDownloadManager.submitAllStoredDownloads.

The submitAllStoredDownloads does several things. It queues up downloads that have been in certain states, i.e. awaiting network. It populates the internal map, the one that is used as our in-memory source of truth for downloads. We then trigger a download for everything stored in this map. Triggering a download will then perform a sequence of steps, for which a DownloadStatus is always emitted, even for Downloaded.

Perhaps it would have been better to split the flow for submitAll and the network recovery, but there is no guarantee that the internal map etc will be present, so we felt it was always better to kick of the initial flow and have the client handle the perceived duplicate events of DOWNLOADED.

Hope that answers your questions 😄 happy to continue the discussion if necessary.

mspmax commented 5 years ago

Hi @Mecharyry,

Thank you so much for your prompt reply. It definitely makes sense from the library point of view but from a client perspective, we would only wanna know about batches which are not download completed during this scenario. any possibility on adding a check on the submitAll to omit the completed states? or maybe add a feature flag which we can configure?. I do understand this can be a tricky issue and I have already hacked our system to achieve this but it's bit ugly and makes me feel bad about it :D

mspmax commented 5 years ago

And one more thing to add I noticed one more thing when the app recovers from a lost network connection it triggers all the notifications including the completed state. Steps to reproduce,

  1. start downloading 3 batches and let it complete within this application lifecycle.
  2. dismiss all the notifications
  3. start one more and let it go to state downloading and remove network connection
  4. then reinstate the network connection
  5. there will be 4 notifications including the earlier completed ones

maybe this is also because submitAll() looks at current memory cache(map)? irrespective of the state

Mecharyry commented 5 years ago

@mspmax thanks for your detailed replies 😄 they are really helpful.

any possibility on adding a check on the submitAll to omit the completed states? or maybe add a feature flag which we can configure

This is a possibility, some work needs to be done around the network recovery in general, so we can certainly look into it. I'm unsure of a timeframe for this but I will try and keep you posted here.

maybe this is also because submitAll() looks at current memory cache(map)? irrespective of the state

This would be certainly due to the submitAll. Again tied to the above, so we need to have a look at it.

I'm going to rope @zegnus and @danybony into this conversation as maintainers too 😄

Thanks again @mspmax

zegnus commented 5 years ago

Hi @mspmax, for now, you can provide your own NotificationCustomizer to the library so that you can decide the type of notification the library should show per event. For example return NotificationDisplayState.HIDDEN_NOTIFICATION will not show it.

.withNotification(new NotificationCustomizer<DownloadBatchStatus>()...

In there you will receive a DownloadBatchStatus where you can check if its completed or not and decide what you want to do.

Mecharyry commented 5 years ago

I just wanted to add further to @zegnus's comment. These issues can be dealt with by client applications by introducing a thin layer on top of the DownloadBatchStatusCallback. For example an implementation of DownloadBatchStatusCallback whose responsibility it is to filter callbacks based on the status.

I have already hacked our system to achieve this but it's bit ugly and makes me feel bad about it :D

I'm assuming you have already done something similar 😂 I realise this is not ideal but it would solve your problem in the short-term. The maintainers will try and keep you updated as we work on the network-recovery of the library.

mspmax commented 5 years ago

Thanks @Mecharyry !