Skip to content
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

DownloadPackge.Storage initialized twice when library begins from local DownloadPackage json file #166

Closed
liuhj1018 opened this issue Aug 27, 2024 · 12 comments · Fixed by #173
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@liuhj1018
Copy link

I download a large file. I put the downloadpackage json file to the disk. When I reboot the progress and resume from the json file, a error shows the datafile is in use. I find line 25 and line 87 same in the ConcurrentStream.cs and I modify the DownloadPackage.cs. The datafile can be downloaded correctly.
image

Is it a bug? or I use in a wrong way?
Look forward for reply!

@liuhj1018
Copy link
Author

if (File.Exists(processFile))
{
string content = File.ReadAllText(processFile);
var newPack = JsonConvert.DeserializeObject(content);
await downloader.DownloadFileTaskAsync(newPack);
}
else
{
await downloader.DownloadFileTaskAsync(url, tempFile);
}

@bezzad
Copy link
Owner

bezzad commented Sep 17, 2024

DownloadPackage

@liuhj1018 When you check the null condition for storage, don't update storage when it was used before. So, when I want to use the Downloader object again to download another URL, I get it wrong because I use an older storage stream and override the old data with new data.

@bezzad
Copy link
Owner

bezzad commented Sep 17, 2024

Do you stop the Downloader process with the Stop() method and then exit from your app or reboot it?
I ask this question because this issue didn't happen before this and when you call the stop, all processes have been done actually and disposed of this cycle. So, you can continue any time you want.

@bezzad bezzad self-assigned this Sep 17, 2024
@bezzad bezzad added the help wanted Extra attention is needed label Sep 17, 2024
@liuhj1018
Copy link
Author

Do you stop the Downloader process with the method and then exit from your app or reboot it? I ask this question because this issue didn't happen before this and when you call the stop, all processes have been done actually and disposed of this cycle. So, you can continue any time you want.Stop()

No, I don't call the stop method. I just simulate the program or the computer has benn suddenly shut down. This is not normal but happen.
I will check null condition for storage, and the current and the next downloader.

@liuhj1018
Copy link
Author

I want to write the Package to the local file using "var packageJson = JsonConvert.SerializeObject(package);". But Where I put the sentence? OnDownloadProgressChanged or OnChunkDownloadProgressChanged method?

@bezzad
Copy link
Owner

bezzad commented Oct 7, 2024

In the case of sudden shutdowns (like a power failure or unexpected program termination), you’ll want to back up the package object, which contains the metadata necessary to resume the download. My Downloader library is designed to resume downloads by serializing the package object as JSON and storing it in a file, database, or another persistent storage. This package can then be passed back to the Downloader to continue the download from where it left off.

When to Serialize and Backup the Package

You’re right to consider the performance impact of serializing the package on every OnProgress event. While backing up on every event would ensure no data is lost, it could indeed be too resource-intensive. To address this, I recommend using a debounce algorithm to limit how often the serialization occurs. This will prevent redundant operations and reduce the performance overhead while ensuring that only the latest package is backed up.

Steps to Take:

  1. Use OnDownloadProgressChanged:

    • This is the appropriate place to serialize and back up the package. However, instead of doing it on every single progress event, debounce the operation. For example, you could serialize the package every few seconds or after a certain amount of data has been downloaded.
  2. Handling Race Conditions:

    • Since your download process is multithreaded, there's a chance of race conditions where multiple threads might try to access or modify the package concurrently. Make sure to implement proper synchronization (e.g., using locks) to avoid corrupting the package during serialization.
  3. Handling Sudden Shutdowns:

    • Since the program might shut down unexpectedly, serializing the package periodically is crucial to ensure you don’t lose progress. While I haven’t tested this exact scenario yet, backing up the package periodically using a debouncing mechanism should provide a good balance between performance and reliability.

Here's a rough idea of how debouncing could be implemented:

private DateTime _lastSerializedTime = DateTime.MinValue;
private readonly TimeSpan _debounceInterval = TimeSpan.FromSeconds(5);

void OnDownloadProgressChanged(object sender, DownloadProgressChangedEventArgs e)
{
    if (DateTime.Now - _lastSerializedTime > _debounceInterval)
    {
        var packageJson = JsonConvert.SerializeObject(package);
        File.WriteAllText("packageBackup.json", packageJson);
        _lastSerializedTime = DateTime.Now;
    }
}

This way, you can ensure the package is backed up regularly without overwhelming the system.
* Serialize the package in the OnDownloadProgressChanged method but apply debouncing to avoid performance issues.
* Be mindful of race conditions in a multi-threaded environment by using synchronization.
* Regular backups of the package object will help recover from sudden shutdowns or crashes without too much data loss.

I haven’t fully tested this particular scenario, but I believe this approach should work for your needs. Let me know if you need any more help with this!

@liuhj1018
Copy link
Author

liuhj1018 commented Oct 8, 2024

two problems:

  1. packageBackup.json write conflict. File.WriteAllTextAsync("packageBackup.json", packageJson);
  2. Restart program, downloading file write conflict. Need add Storage null judgment in the DownloadPackage.cs.
    image

@bezzad
Copy link
Owner

bezzad commented Oct 14, 2024

I don't Undrestand the right issue. According to the previews answer you must handle race conditions by debouncing and locking objects to prevent write conflict issues. please explain more about what happened.

@liuhj1018
Copy link
Author

1.packageBackup.json write conflict. I had used locking object method first, but its efficiency was low. So now I use File.WriteAllTextAsync method.
2.When I generate the local backup.json file, shutdown the program and restart, the file stream is initialized in the ConcurrentStream.cs (line 31) firstly, and then initialized in the DownloadPackage.cs (line 138) secondly. They are conflict. I add Storage nullvalue judgement to avoid initializing twice on the same file.

@ktxiaok
Copy link

ktxiaok commented Nov 10, 2024

Hi @bezzad , I think the cause of this problem is that the DownloadPackage.Storage is serialized into JSON. When the JSON file is deserialized, a storage instance will be created which conflicts with the instance created by DownloadPackage.BuildStorage later. I think the property DownloadPackage.Storage isn't supposed to be serialized. It should have an attribute [NonSerialized].

@bezzad
Copy link
Owner

bezzad commented Nov 16, 2024

Sorry for the delayed response; I've been quite busy these past few weeks.
Thanks for your patience—I'm working on it, and I’ll keep you updated with any progress or solutions.

@bezzad bezzad added the bug Something isn't working label Nov 17, 2024
@bezzad bezzad moved this to In progress in Downloader Nov 17, 2024
@bezzad bezzad linked a pull request Nov 20, 2024 that will close this issue
@bezzad
Copy link
Owner

bezzad commented Nov 20, 2024

Hi @liuhj1018 @ktxiaok

Thank you for bringing this issue to our attention! We've made several updates and improvements to the Downloader package. Please test the newly released v3.3.0 and let us know if you're still encountering the issue.
Your feedback is incredibly valuable to us, and we appreciate your patience and support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
Status: In progress
Development

Successfully merging a pull request may close this issue.

3 participants