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

Overall progress for paused torrents is sometimes mis-reported - recheck fixes it #12227

Open
FranciscoPombal opened this issue Mar 23, 2020 · 5 comments

Comments

@FranciscoPombal
Copy link
Member

@FranciscoPombal FranciscoPombal commented Mar 23, 2020

Please provide the following information

qBittorrent version and Operating System

Latest master, Ubuntu 18.04

If on linux, libtorrent-rasterbar and Qt version

libtorrent 1.2.5, Qt 5.9.5

What is the problem

Overall progress is sometimes misreported (i.e. inconsistent with file progress) for paused torrents. This is probably some kind of race-condition.

I have observed this with fast torrents (e.g. Ubuntu ISO torrents), in which case the overall progress should be the same as the file progress. A force recheck will fix the problem. When it differs, the overall progress is always greater than file progress. I have not seen any instance where it was less.

What is the expected behavior

Overall progress should always be accurate.

Steps to reproduce

  1. This is probably easier to reproduce on a fast single-file torrent.
  2. Start downloading torrent
  3. Pause it before it finishes
  4. Observe the inconsistency in reported progress - it could be something like 0.9% overall, 0.6% for the file.
  5. Force recheck torrent
  6. Observe that overall progress is now consistent with file progress.

Extra info(if any)

This probably also happens in multi-file torrents, but it's harder to see if the overall progress percentage is correct or not.

@glassez

This comment was marked as off-topic.

Copy link
Member

@glassez glassez commented Mar 23, 2020

@FranciscoPombal, it seems more logical if Confirmed bug label is assigned by someone other than the issue reporter. Otherwise, it looks useless.

@FranciscoPombal

This comment was marked as off-topic.

Copy link
Member Author

@FranciscoPombal FranciscoPombal commented Mar 23, 2020

@FranciscoPombal, it seems more logical if Confirmed bug label is assigned by someone other than the issue reporter. Otherwise, it looks useless.

This label assignment can always be challenged. IMO it's reasonable to have bugs reported by a team member automatically become "Confirmed bugs" and remain so until proven otherwise, and not the other way around.

If you need more information for reproducing this let me know. I imagine for this one it will be tricky, as reproducing, diagnosing and fixing race condition-type bugs is always a pain.

@thalieht

This comment has been minimized.

Copy link
Contributor

@thalieht thalieht commented Mar 23, 2020

I confirm.

@glassez

This comment has been minimized.

Copy link
Member

@glassez glassez commented Mar 24, 2020

Well, I can confirm this issue (torrent isn't required to be paused to reproduce it).
Seems it is due the less accurate progress calculations for individual files.

m_nativeHandle.file_progress(fp, lt::torrent_handle::piece_granularity);

@FranciscoPombal

This comment has been minimized.

Copy link
Member Author

@FranciscoPombal FranciscoPombal commented Mar 26, 2020

@glassez

Well, I can confirm this issue (torrent isn't required to be paused to reproduce it).
Seems it is due the less accurate progress calculations for individual files.

m_nativeHandle.file_progress(fp, lt::torrent_handle::piece_granularity);

lt::torrent_handle::piece_granularity is a flag to make the call to file_progress() cheaper. But it has another advantage. The docs say:

only calculate file progress at piece granularity. This makes the file_progress() call cheaper and also only takes bytes that have passed the hash check into account, so progress cannot regress in this mode.

This corroborates my observation that file progress is always the smaller value and never regresses. I also think the expected user experience is to not make progress regress.

Then I took a look at other places in the code and found that, for example, folder progress in the "Content" tab is derived from summing all file progress under it. This makes sense.

But then I saw the code for the global progress:

qreal TorrentHandle::progress() const

It is not based on file progress, hence the disparity. In addition, the logic is not entirely clear to me, why does it not always return m_nativeStatus.progress;? What is the reason for the other cases?

And this leads to even more questions:

  1. Why does the file/folder progress calculation in the Content tab not require similar isChecking() case distinctions?
  2. If there were no additional disadvantages to just returning m_nativeStatus.progress; (torrent_handle::status().progress in libtorrent) for the global progress, would it still differ from file progress?

If the answer to 2 is "yes", then we should consider calculating total progress from file progress as well. It is probably even "free" since we are already iterating through all file progress elsewhere to populate the content tab. But in this case I'd think it would make more sense for total progress reported by libtorrent to be consistent with file progress, independently of the value of lt::torrent_handle::piece_granularity. @arvidn Thoughts on this last paragraph?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.