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

Fix stop tracker timeout issue on high latency connections. #11807

Closed
wants to merge 1 commit into from
Closed

Fix stop tracker timeout issue on high latency connections. #11807

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 2, 2020

By default libtorrent uses a 5 second timeout for stop event.
But qBit is using a 1 second timeout for stop event (when you pause a torrent or close the client).
It's a very low default for a high latency connection. I was getting tracker timeout issue everytime I paused a torrent on a particular tracker with a 350 ms latency. Never had issues with annoucing to that tracker. Only stop events caused the timeout. Turns out the default announce timeout is much higher (10 seconds) in libtorrent and qBit doesn't alter that. It only alters the stop_tracker timeout values. So setting it to the default 5 second is a reasonable value and should prevent tracker timeouts on high latency connections.

By default libtorrent uses a 5 second timeout for stop event.
But qBit is using a 1 second timeout for stop event (when you pause a torrent or close the client).
It's a very low default for a high latency connection. I was getting tracker timeout issue everytime I paused a torrent on a particular tracker with a 350 ms latency. Never had issues with annoucing to that tracker. Only stop events caused the timeout. Turns out the default announce timeout is much higher (10 seconds) in libtorrent and qBit doesn't alter that. It only alters the stop_tracker timeout values. So setting it to the default 5 second is a reasonable value and should prevent tracker timeouts on high latency connections.
@FranciscoPombal
Copy link
Member

FranciscoPombal commented Jan 2, 2020

From the libtorrent docs:

This is usually shorter, to make the client quit faster.

I guess this was the original reasoning for setting it so short in qBittorrent (even shorter than the libtorrent default). Most people who care about increasing this timeout are probably private tracker users where it is beneficial to have all the important events properly accounted for, even if that means waiting a long time for the client to exit.

As such, I think this should be a configurable setting, and the current default value left as-is. Otherwise, the average user will just get annoyed by the extra 4 second delay of qBittorrent exiting in case there is a single malfunctioning tracker.

The private tracker audience tends to be more tech savvy and has no problem following guides to optimize qBittorrent settings for their use case. The important thing is having the ability to tune the settings.

Another example: There is an ongoing discussion about making qBIttorrent capable of racing. While me and many users would love for the feature to be available, me (and I think most people) agree that racing-optimized settings should not be turned on by default.

In general, I guess the underlying philosophy is: "have sane defaults for the average/common case, but also the option to tune the dials and knobs for specific/power user use cases".

So can you change the scope of this PR to expose this as a setting instead? If you do so, be careful to check whether or not changing this requires a restart of the client to take effect (I don't think it does, but please verify) and don't forget about the WebUI.

@ghost
Copy link
Author

ghost commented Jan 2, 2020

From the libtorrent docs:

This is usually shorter, to make the client quit faster.

I guess this was the original reasoning for setting it so short in qBittorrent (even shorter than the libtorrent default). Most people who care about increasing this timeout are probably private tracker users where it is beneficial to have all the important events properly accounted for, even if that means waiting a long time for the client to exit.

As such, I think this should be a configurable setting, and the current default value left as-is. Otherwise, the average user will just get annoyed by the extra 4 second delay of qBittorrent exiting in case there is a single malfunctioning tracker.

The private tracker audience tends to be more tech savvy and has no problem following guides to optimize qBittorrent settings for their use case. The important thing is having the ability to tune the settings.

Another example: There is an ongoing discussion about making qBIttorrent capable of racing. While me and many users would love for the feature to be available, me (and I think most people) agree that racing-optimized settings should not be turned on by default.

In general, I guess the underlying philosophy is: "have sane defaults for the average/common case, but also the option to tune the dials and knobs for specific/power user use cases".

So can you change the scope of this PR to expose this as a setting instead? If you do so, be careful to check whether or not changing this requires a restart of the client to take effect (I don't think it does, but please verify) and don't forget about the WebUI.

Well, it doesn't cause any delay in the closing of the client even with malfunctioning tracker. The UI closes instantly. The process usually takes 2-3 seconds to exit after the UI closes even with a timeout set to 1. And with this change(5 second) I didn't find any signifcant delay in the closing of the process except maybe an extra second or two. The UI as usual closes instantly so I don't see why user would complain?
Exposing the settings could be done in separate PR. This was just about the default low settings for the timeout.

@FranciscoPombal
Copy link
Member

FranciscoPombal commented Jan 2, 2020

Well, it doesn't cause any delay in the closing of the client even with malfunctioning tracker. The UI closes instantly. The process usually takes 2-3 seconds to exit after the UI closes even with a timeout set to 1. And with this change(5 second) I didn't find any signifcant delay in the closing of the process except maybe an extra second or two. The UI as usual closes instantly so I don't see why user would complain?
Exposing the settings could be done in separate PR. This was just about the default low settings for the timeout.

It is not enough for the UI to close instantly. In fact, the user is worse off if the UI closes fast and then the process takes a long time to do so, because then the user has no way of knowing the process is still up unless they check their task manager. The process itself should also close ASAP, as it will either prevent qBittorrent from being restarted while it is still active or at worse cause corruption of settings and/or fastresumes.

Closing tracker tracker connections is not the only thing that qBittorrent does when closing of course, but the fact that qBittorrent already takes some seconds to take care of other things is no excuse to have another 4 seconds added to closing time, worst-case. And this last point is imporntant; in these kinds of things you should be thinking about worst-case scenario, not average. If your only concern were average case, why not increase the timeout to 10, 30 or even 60 seconds? After all, it almost never happens, on average.

@ghost ghost closed this Jan 2, 2020
@ghost ghost deleted the an0n666-stop-tracker-fix branch January 2, 2020 17:42
@FranciscoPombal
Copy link
Member

So are you going to open another PR exposing the stop_tracker_timeout as a configurable advanced setting or not?

@ghost
Copy link
Author

ghost commented Jan 4, 2020

So are you going to open another PR exposing the stop_tracker_timeout as a configurable advanced setting or not?

A 1 second timeout is not a sane value anyways. Whereas qBit takes 3-4 seconds to close on a SSD system, a sane value would be atleast 3-5 seconds. This timeout doesn't "add" to the already "slow" closing of qBit btw. Infact the timeout might be reached even before the "slow" qbit process exits. So the worst case scenarios that you were talking about could already be happending for many users due to the "slow" closing of the process..

@FranciscoPombal
Copy link
Member

This timeout doesn't "add" to the already "slow" closing of qBit btw.

I realize that it might not be the case that the timeout adds to whatever else qbittorrent does on exit, since that stuff may be asynchronous with respect to the stop tracker timeout. But, to be sure, we need data. Specifically, some kind of profiling data to validate these kinds of statements.

1 second timeout is not a sane value anyways.

Regardless of your opinion of what is and isn't a sane value for this setting, I do agree with the fact that it is not ideal for all use cases. That is why it should be configurable. So that is why I encourage you to submit a PR exposing this as an advanced setting.

Anyway @glassez @Chocobo1 what do you think about the default value?

@ghost
Copy link
Author

ghost commented Jan 5, 2020

This timeout doesn't "add" to the already "slow" closing of qBit btw.

I realize that it might not be the case that the timeout adds to whatever else qbittorrent does on exit, since that stuff may be asynchronous with respect to the stop tracker timeout. But, to be sure, we need data. Specifically, some kind of profiling data to validate these kinds of statements.

1 second timeout is not a sane value anyways.

Regardless of your opinion of what is and isn't a sane value for this setting, I do agree with the fact that it is not ideal for all use cases. That is why it should be configurable. So that is why I encourage you to submit a PR exposing this as an advanced setting.

Anyway @glassez @Chocobo1 what do you think about the default value?

There are some things which should be "working" out of the box and then you have to think if it should be configurable or not. If it's configurable, people that have this sort of issue could probably set a much higher limit as well. But here the concern is regarding the out of the box situation. 1s timeout does not work for a lot of use cases. People probably do not notice that because it happens under the hood. Specially in countries where people have a high latency to tracker or when the tracker is slow responding. Also in case of SSL trackers it takes about 250 ms to half a second to just perform a handshake. Now add a 300 ms latency delay in sending the handshake and then another 300ms for the GET request(stop announce). Your 1 second is gone by that time.

I think @arvidn can give a better input.

@arvidn
Copy link
Contributor

arvidn commented Jan 6, 2020

My suspicion is that setting the timeout to 1 does not help the shutdown hang issue. My impression is that multiple synchronous DNS lookups, running in serial, is the main contributor (short of bugs)

@FranciscoPombal
Copy link
Member

@arvidn

My impression is that multiple synchronous DNS lookups, running in serial, is the main contributor (short of bugs)

And would this be a libtorrent or a qBIttorrent issue? Does libtorrent expose a way of doing the lookups asynchronously?

@arvidn
Copy link
Contributor

arvidn commented Jan 6, 2020

And would this be a libtorrent or a qBIttorrent issue? Does libtorrent expose a way of doing the lookups asynchronously?

it might depend on who you ask :) It's not a qBittorrent issue. libtorrent issues asynchronous DNS requests with asio, but most operating systems don't expose a way to perform them asynchronously, so asio runs them in serial on a separate thread.

one problem is that you can't (afaik) abort a synchronous DNS lookup that's stuck, so you get a very long timeout. in newer versions of libtorrent I've tried to mitigate this by caching the DNS response and, specifically at shutdown, force-use the cached IP for trackers. I think it would improve the situation in most cases at least

@FranciscoPombal
Copy link
Member

@arvidn

it might depend on who you ask :) It's not a qBittorrent issue. libtorrent issues asynchronous DNS requests with asio, but most operating systems don't expose a way to perform them asynchronously, so asio runs them in serial on a separate thread.

one problem is that you can't (afaik) abort a synchronous DNS lookup that's stuck, so you get a very long timeout. in newer versions of libtorrent I've tried to mitigate this by caching the DNS response and, specifically at shutdown, force-use the cached IP for trackers. I think it would improve the situation in most cases at least

So what is your opinion about changing this default? Should it be the same as the libtorrent default out of the box (5 seconds)? As stated above, my main concern is whether this would add significantly to the shutdown time, besides the stuff like dns lookups and such.

@arvidn
Copy link
Contributor

arvidn commented Jan 6, 2020

I think 5 seconds is reasonable. But I suspect it very rarely even matters (to make the stop-timeout 5, instead of the normal timeout of 30 seconds, iirc)

@FranciscoPombal
Copy link
Member

@arvidn

I think 5 seconds is reasonable. But I suspect it very rarely even matters (to make the stop-timeout 5, instead of the normal timeout of 30 seconds, iirc)

For the possibility of fine tuning, #11834 is being worked on.

@glassez
Copy link
Member

glassez commented Jan 8, 2020

@arvidn, some users reported there is a problem whith unreachable tracker names (i.e. dns takes long time when trying to resolve it). I never debug it but it seems weird for me if libtorrent try to send stop event to previously unreached (when it sends announce event) trackers...

@arvidn
Copy link
Contributor

arvidn commented Jan 8, 2020

libtorrent does retry trackers that fail, (almost) regardless of the reason they failed. a hostname lookup may fail today, but work tomorrow. A hostname that doesn't exist doesn't necessarily cause a problem. The DNS server will just respond with a failure immediately. My impression is that the problem with stalling happens when the internet connectivity or the DNS server is broken or flaky. That will result in no response to hostname queries, and stall.

@glassez
Copy link
Member

glassez commented Jan 8, 2020

libtorrent does retry trackers that fail, (almost) regardless of the reason they failed. a hostname lookup may fail today, but work tomorrow.

Yes. It is reasonable for announcing. But what if some tracker was never touched during current session (due to its name unreachability or some other reason)? Does libtorrent send "stop" event to it?

@arvidn
Copy link
Contributor

arvidn commented Jan 8, 2020

I'm not sure. I think the state of "started sent" is kept around, and if that's false, it would make sense to omit an event=stopped announce to that tracker

@arvidn
Copy link
Contributor

arvidn commented Jan 8, 2020

@glassez something like this arvidn/libtorrent#4236
Would you mind giving it a try?

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

Successfully merging this pull request may close these issues.

3 participants