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

Announce to all trackers if IP changed #15001

Conversation

zhuangzi926
Copy link
Contributor

Update

2021.05.22

2021.05.20

  • Remove redundant codes and fix coding style
  • Rename *ReannounceWhenAddressChanged to *ReannounceWhenAddressChangedEnabled

2021.05.18

  • Relocate methods in session.h and session.cpp
  • Fix coding styles in session.cpp
  • Change m_lastExternalIP from QHostname to QString
  • Extract duplicated reannounce loops into a single class method
  • Relocate declarations in advancedsettings.h, advancedsettings.cpp and appcontroller.cpp
  • Remove redundant urls in appcontroller.cpp

Motivation

Try to implement this feature request: issue #14545.

Considering many users run qbt behind NATs, it's quiet cumbersome for them to tolerate frequent torrent not available issue because of public IP change.

So I came up with this direct idea to regularly detect public ip catch external IP change alert from libtorrent, and announce it to all trackers if change happens.

Brief of this PR

  • Add a timer in BitTorrent::Session to send a public ip address request to http://api64.ipify.org every 30 mins.
  • Once public ip address change is detected, force announce to all trackers.
  • Once external IP change alert is captured or our user deliberately sets a new listening port, reannounce to all trackers without delay.
  • This new feature is optional, and can be toggled during running time, default is false.
  • GUI checkboxes in advanced setting on both desktop and web ui are included.
  • Update dyndns service register url to the latest one.

About external/public IP detection

Although it's known that libtorrent can get extern ip through communications with trackers and peers, it seems that libtorrent only catches this ip change alert but not try to reannounce to improve efficiency. Moreover, the interval of external ip detection behavior by libtorrent is uncertain and unpredicatable(to some extent) for downstream apps like qbt, it would probably be better to implement this logic by qbt itself.

I have found identical codes in src/base/net/dnsupdater.cpp, which uses a timer to ask public dns provider to update domain-ip binding if ip changed. Reimplemention of this logic in BitTorrent::Session might introduce incompatibility problems, which may need assistance from professional reviewers.

It is now known that libtorrent catches external IP change alert in an interval of 5 minutes, and qbt regularly reannounce(if ignore tracker settings) every 30 minutes. So it would be better if we implement this feature request by adding an option to force reannounce when external IP or listening port changes(before this PR qbt just catches these alerts but sends nothing). There is no need to introduce a new QTimer or external IP request from another third party.

Control this feature by GUI users

GUI control to set ip detection interval and switch on/off this new feature are needed for our end-users.

I'm not good at coping with codes in src/gui, hoping other contributors might help with this new feature.

GUI checkboxes for both desktop and web ui are implemented following 5161758.

zhuangzi926 and others added 30 commits March 28, 2021 18:19
- Add a timer in BitTorrent::Session to send a public ip address request to http://api64.ipify.org every 30 mins.
- Once public ip address change is detected, force announce to all trackers.
- This new feature is optional, can be toggled during running time, default is false.
…ublic-ip-change-detected' into pr-announce-to-all-trackers-if-public-ip-change-detected

# Conflicts:
#	src/base/bittorrent/session.cpp
#	src/base/bittorrent/session.h
#	src/gui/advancedsettings.cpp
#	src/gui/advancedsettings.h
@zhuangzi926
Copy link
Contributor Author

@Chocobo1 @thalieht @glassez I reopened #14650 in this new PR.

I tried to git rebase to squash all commits, but accidentally closed the old PR...

Copy link
Member

@glassez glassez left a comment

Choose a reason for hiding this comment

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

Last comment from me.

.arg(toString(p->external_address)), Log::INFO);
.arg(externalIP), Log::INFO);

if (isReannounceWhenAddressChangedEnabled() && !m_lastExternalIP.isEmpty() && m_lastExternalIP != externalIP)
Copy link
Member

Choose a reason for hiding this comment

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

I would rewrite it as the following :

if (m_lastExternalIP != externalIP)
{
    if (!m_lastExternalIP.isEmpty() && isReannounceWhenAddressChangedEnabled())
        reannounceToAllTrackers();
    m_lastExternalIP = externalIP;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@glassez glassez changed the title Reopen: Announce to all trackers if IP changed Announce to all trackers if IP changed May 22, 2021
@glassez glassez requested a review from Chocobo1 May 22, 2021 11:18
@glassez glassez added the Core label May 22, 2021
@glassez glassez added this to the 4.4.0 milestone May 22, 2021
@FranciscoPombal
Copy link
Member

Shouldn't some of these commits be squashed @glassez @Chocobo1? Otherwise, LGTM.

@glassez
Copy link
Member

glassez commented May 22, 2021

Shouldn't some of these commits be squashed @glassez @Chocobo1?

Of course, it makes sense to have only one commit here. But as I understand it, the author has a problem with this. Fortunately, we recently changed the merge policy so that it can/will be squashed when merged.

@FranciscoPombal
Copy link
Member

Of course, it makes sense to have only one commit here. But as I understand it, the author has a problem with this. Fortunately, we recently changed the merge policy so that it can/will be squashed when merged.

Cool, just checking. I got a little scared after seeing your approval already, but now it makes sense :)

@Chocobo1 Chocobo1 merged commit 2e8e2b0 into qbittorrent:master May 23, 2021
@Chocobo1
Copy link
Member

@zhuangzi926
Thank you!

IceCodeNew pushed a commit to IceCodeNew/qBittorrent-Enhanced-Edition that referenced this pull request May 30, 2021
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.

4 participants