BuIlDaLiBlE / BetterHI3Launcher

A much better Honkai Impact 3rd launcher.

The Unlicense

102 stars 21 forks source link

Rewrite Parallel Download #51

Closed neon-nyan closed 2 years ago

neon-nyan commented 2 years ago

Pull Request Overview

This Pull Request is supposed to rewrite the entire inner HttpClient class and fix these problems:

File lock issues while:
- Resuming/Pausing the download really fast.
- Merging file and causes the launcher to crash.
- Retrying download at some chunks.

It also bring some minor changes while cancelling the download. For now, DisposeAndWait() is introduced and will be called instead of Dispose(). This will prevent the download to not cancelled properly and make the file getting locked while threads of the chunks are actually still running before it's completely cancelled

Major Changes

Rewrite the entire Inner HttpClient class.
Introducing DisposeAndWait() in DownloadParallelAdapter class to dispose and wait the thread to be completely cancelled.

Minor Changes

Check for Infinity number on DownloadStatusChanged event while there's no data received.

BuIlDaLiBlE commented 2 years ago

LGTM! I'm going to merge soon, so pls tell if there's anything else you'd like to add here.

neon-nyan commented 2 years ago

As discussed in discord, I have something to test first before this PR can be merged. I will let you know once the test run successfully.

neon-nyan commented 2 years ago

I think that's all for now. Let me know if there's anything odd with it.

BuIlDaLiBlE commented 2 years ago

There's something weird going on with pause again. Try pausing and then clicking on resume button before the download actually resumes, the download progress bounces back and UI can even hang for quite a while.

neon-nyan commented 2 years ago

Is it on pre-download or normal download? Also, that might be better to send the screen capture as well

BuIlDaLiBlE commented 2 years ago

Just a normal download. Sorry, can't screen capture but it's pretty easy to replicate.

neon-nyan commented 2 years ago

Okay, I just made an investigation. Unfortunately, it's expected and it's kind of seamless minor issue for me. It bounces back sometimes because all threads weren't ready to continue the session. While it's trying to resume, at the same time two things happen:

Waiting for the HttpResponseMessage to be done.
After that, it will check the size of the existing size of chunks (if any) and increase it to _DownloadedSize

However, point no.2 cannot be moved to the first place because It also need StartOffset and EndOffset from TryGetContentLength() before those offset can be calculated. I can make it synchronized by adding a check to each threads and will only calculate the existing size when all threads are ready. But it's too risky because it will causes other bugs like file access lock, data misalignment and thread lock related issues. Since all threads need to be running asynchronously.

But if you have any idea, that would be great.

BuIlDaLiBlE commented 2 years ago

Hmm, I think making the pause button not available (or just hide it) for this small period of time is better in this case then. I can make the thread sleep but I don't really like to do that. Can I get the status of downloader that it stopped fully so I can check that and make the button appear?

neon-nyan commented 2 years ago

Agreed. Well, you can actually do it by replacing Stop() with async await StopAndWait() method. This will make the process to wait until all threads stopped.

You can hide the button after await StopAndWait() has finished.

BuIlDaLiBlE commented 2 years ago

Okay I'll do some testing and then merge then! Otsukaresama!

neon-nyan commented 2 years ago

Alright, nice to work with you. Just let me know if you need other things again.