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

Tracker is errored only if all local endpoints fail #11733

Merged

Conversation

sledgehammer999
Copy link
Member

Closes #11691

@sledgehammer999 sledgehammer999 changed the title Tracker errored only if all local endpoints fail Tracker is errored only if all local endpoints fail Dec 22, 2019
src/base/bittorrent/torrenthandle.cpp Outdated Show resolved Hide resolved
src/base/bittorrent/torrenthandle.cpp Outdated Show resolved Hide resolved
@Chocobo1 Chocobo1 added this to the 4.2.2 milestone Dec 23, 2019
@Chocobo1 Chocobo1 added the Core label Dec 23, 2019
Chocobo1
Chocobo1 previously approved these changes Dec 23, 2019
Copy link
Member

@Chocobo1 Chocobo1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, always found out something after pressing the approve button :\

src/base/bittorrent/torrenthandle.cpp Outdated Show resolved Hide resolved
m_session->handleTorrentTrackerError(this, trackerUrl);
// Starting with libtorrent 1.2.x each tracker has multiple local endpoints from which
// an announce is attempted. Some endpoints might succeed while others might fail.
// Emit the signal only if all endpoints have failed. TrackerEntry::isWorking() returns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment of how TrackerEntry::isWorking() works should be documented at trackerentry.cpp, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I did it here to help any reader on why I seem to check only once but I say "multiple endpoints" (in the comment).

@sledgehammer999
Copy link
Member Author

Ok, I resolved the issue with isWorking() in another way. Take another look.

Chocobo1
Chocobo1 previously approved these changes Dec 24, 2019
glassez
glassez previously approved these changes Dec 24, 2019
@sledgehammer999
Copy link
Member Author

@glassez @Chocobo1 please repeat your approval.
Now I changed it to use the find_if() algo.
Havign the same tracker (url+port) on different tiers is nonsensical. Furthermore, the tracker alert doesnt' allow to distinguish a tracker tier. Only a tracker url (which includes port too).

Chocobo1
Chocobo1 previously approved these changes Jan 3, 2020
src/base/bittorrent/torrenthandle.cpp Outdated Show resolved Hide resolved
@sledgehammer999 sledgehammer999 merged commit 45ed31f into qbittorrent:master Jan 8, 2020
@sledgehammer999 sledgehammer999 deleted the tracker_error_count branch January 8, 2020 02:24
@kasper93
Copy link
Contributor

kasper93 commented Mar 5, 2020

@sledgehammer999 I tested build you posted in related issue and it doesn't work well. It fixes the issue described, but when a tracker returns error message, this error message is not shown, at least not always.

Looking at the code it is quite clear why this happens. I'm yet to build and debug this, but let me explain what I think happens.

Quick recap so we are on the same page, correct me if I mixed up something.

  1. libtorrent creates multiple endpoints for single tracker
  2. libtorrent sends tracker_error_alert in case of error on any endpoint for given tracker
  3. qbt ignores this alert if any other endpoint reports to be working

Now this logic is fine, but look at the line here
https://github.com/qbittorrent/qBittorrent/blob/24cd7c3/src/base/bittorrent/torrenthandle.cpp#L1632
message is unconditionally overwritten. Even if the alert is not pass through.

I have a case, where tracker returns "unregistered torrent", but the message in qbt is blank (works fine with 4.2.0). Probably because if was overwritten, by the error on the other endpoint, which failed to connect of smth. Anyhow, I would need to debug this to tell more, but maybe you will fix it sooner :)

@ghost
Copy link

ghost commented Mar 5, 2020

I have a case, where tracker returns "unregistered torrent", but the message in qbt is blank (works fine with 4.2.0). Probably because if was overwritten, by the error on the other endpoint, which failed to connect of smth. Anyhow, I would need to debug this to tell more, but maybe you will fix it sooner :)

I couldn't reproduce this. Local endpoints error out first..so the last message to overwrite would be the reachable one.

@kasper93
Copy link
Contributor

kasper93 commented Mar 5, 2020

This condition doesn't seem to be guaranteed, does it?

@ghost
Copy link

ghost commented Mar 8, 2020

This condition doesn't seem to be guaranteed, does it?

You're right. It's not guaranteed. However you can't store multiple msgs for each endpoint and display multiple msgs. That would be unreasonable.

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

Successfully merging this pull request may close these issues.

New Bug - v4.2.1 - All Trackers Listed as Error
4 participants