Opened 13 years ago

Closed 9 years ago

Last modified 8 years ago

#1079 closed Bug (fixed)

Webseeds do not respect speed limits

Reported by: Waldorf Owned by: charles
Priority: Low Milestone: 2.80
Component: libtransmission Version: 1.30
Severity: Normal Keywords: patch-needed
Cc: jeff.raber@…, federicoleva@…

Description

Opened a new ticket as per charles recommendation.

Thinking-out-loud, I see two possible ways of doing this.:

(1) we could use libcurl's CURLOPT_MAX_SEND_SPEED_LARGE and CURLOPT_MAX_RECV_SPEED_LARGE functions for this

(2) we could put the webseeds on a different pulse timer than the tracker web pulses, s.t. we could simply pulse less frequently as we got closer to the speed limit.

Neither of these is a shoo-in. At the core of the problem, we don't have the fine-grained control over webseeds like we do peers.

(3) we could set CURLOPT_BUFFERSIZE to request the data in smaller chunks. A combination of this and (2) would do the trick.

Attachments (4)

pipefunctions.patch (8.1 KB) - added by alexat 10 years ago.
web.patch (11.5 KB) - added by alexat 10 years ago.
webseed_bandwidth.patch (27.1 KB) - added by alexat 10 years ago.
web.c (17.7 KB) - added by vipjml 9 years ago.
use this "web.c" can fix this problen, this web.c is modified based on 2.77

Download all attachments as: .zip

Change History (34)

comment:1 Changed 13 years ago by charles

  • Milestone changed from None Set to Sometime

comment:2 Changed 13 years ago by charles

  • Version changed from 1.22+ to 1.30

comment:3 Changed 13 years ago by charles

  • Priority changed from Normal to Low

comment:4 Changed 12 years ago by charles

  • Resolution set to wontfix
  • Status changed from new to closed

There is only one developer currently working on libtransmission, and webseeds are a low priority because webseed .torrents are exceedingly rare. Unless they catch on more in the BitTorrent? world, this ticket is unlikely to be implemented.

This ticket would be an improvement to Transmission, at least to the extent that webseeds are used, so patches are welcomed. If someone out there feels strongly about webseeds, please attach a patch to this ticket and reopen the ticket. Thanks!

comment:5 Changed 12 years ago by livings124

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Reopening because this is a valid issue that should be fixed sometime or other.

comment:6 Changed 12 years ago by charles

  • Keywords patch-needed added

comment:7 Changed 12 years ago by charles

  • Summary changed from Webseeds do not respect download rate limit to Webseeds do not respect speed limits

comment:8 Changed 11 years ago by jordan

  • Milestone changed from Sometime to None Set

comment:9 Changed 10 years ago by alexat

Hi!

I have a concept of solving this. A bit complex, but it should work. However the last problem I'm pondering about is thread safety when doing tr_bandwidth calls from the web thread. Both making the tr_bandwidth interface thread safe and passing these function calls into the right thread aren't really satisfying, so my preferred solution would be to just let go of the second thread for web tasks. I would integrate libcurl into libevent using the curl_multi_socket interface. Would that be OK with you guys? Are there convincing reasons for using a different thread for libcurl? If not I'll develop a patch on this.

Thanks for your explanations!

comment:10 Changed 10 years ago by jordan

That was my initial approach too. I agree that keeping libcurl in a separate thread is awkward. We do it that way now because of stability problems with the multisocket API in past versions of OS X.

I haven't tried it with OS X 10.7; maybe these issues are resolved now.

comment:11 Changed 10 years ago by alexat

Found it: #2987. Apparently this isn't a good idea, I'll think of something else. Thank you!

Changed 10 years ago by alexat

Changed 10 years ago by alexat

Changed 10 years ago by alexat

comment:12 Changed 10 years ago by alexat

So here finally is my proposed patch :) It works pretty well for me but I didn't yet have the time to really test it properly, especially when collaborating with other transfers.

web.patch introduces the new tr_webPauseTask() function and implements a pipe to be able to pass on pause and unpause commands without delay. For this to work pipefunctions.patch moves some functions from trevent.c to platform.c and makes them public.

webseed_bandwidth.patch introduces locking on the bandwidth tree and contains the actual patch. In principle it works by stopping the web task using curl_easy_pause() as soon as the bandwidth limit is reached. I had a look at the source of curl - curl_easy_pause() basically doesn't do anything else than setting a flag on the easy handle, so it shouldn't cause great performance issues to use it for this purpose.

I removed lines 323-341 in https://trac.transmissionbt.com/browser/trunk/libtransmission/bandwidth.c#L323 as there now is a more accurate method of compensating it when the bandwidth limit has been exceeded. Please put them back in if I got their meaning wrong.

Btw there was a bug in libevent that caused measuring bandwidth and the amount of downloaded data from webseeds to become inaccurate: http://sourceforge.net/tracker/?func=detail&aid=3414510&group_id=50884&atid=461322. Still a problem with the current stable release of libevent...

comment:13 Changed 10 years ago by livings124

The Mac version builds against libevent trunk, so it has that bug fix.

comment:14 Changed 10 years ago by jordan

  • Milestone changed from None Set to 2.50

comment:15 follow-up: Changed 10 years ago by jordan

Nice patch & nice explanation.

bandwidth.c lines 323-341 are useful in some swarms even if webseeds aren't involved, so I'm going to leave them in. That scaled throttling helps keep libtransmission's speeds closer to the speed limit.

If you're interested, last year I did a little benchmarking on this with gnome's system monitor and various BitTorrent? clients. Each screenshot here shows a different client downloading the same Ubuntu ISO torrent with speed capped at 15 KB/s both ways, next to a gnome-system-monitor showing overall network use. As a visual aid I added a horizontal green line showing the "ideal" of where a constant 15 KB/s would lie on each g-s-m network graph.

https://launchpadlibrarian.net/55888773/deluge-capped-at-15-up-15-down.png

https://launchpadlibrarian.net/55888856/ktorrent-capped-at-15-up-15-down.png

https://launchpadlibrarian.net/55888861/monsoon-capped-at-15-up-15-down.png

https://launchpadlibrarian.net/55888870/transmission-capped-at-15-up-15-down.png

https://launchpadlibrarian.net/55888935/vuze-capped-at-15-up-15-down.png

comment:16 in reply to: ↑ 15 Changed 10 years ago by alexat

That's interesting, the other clients don't seem to take great care about the speed limit :)

Replying to jordan:

bandwidth.c lines 323-341 are useful in some swarms even if webseeds aren't involved, so I'm going to leave them in. That scaled throttling helps keep libtransmission's speeds closer to the speed limit.

Could you maybe explain that a bit further? The only situation I can think of in which the rate limit could be exceeded (without webseeds) is the following:

  • Request data via µTP
  • Download other data and reach the speed limit
  • Receive the data that was requested via µTP

In this situation the exceeding bandwidth will now be 'borrowed' from the next period of time, so a very exact average speed should be achieved. Did I forget something? It appears to me that this this throttling keeps the used bandwidth below what would be available and/or causes larger overhead because of smaller packets.

comment:17 Changed 10 years ago by jordan

  • Milestone changed from 2.50 to Sometime

comment:18 Changed 9 years ago by jraber

  • Cc jeff.raber@… added

comment:19 Changed 9 years ago by sswam

I noticed this with the Humble Indie Bundle torrents. I set the speed limit to 50 KB/sec and it's still downloading at nearly 1 MB/s! The HIB does have a lot of customers, maybe it's a good reason to fix this.

comment:20 Changed 9 years ago by Nemo_bis

  • Cc federicoleva@… added

comment:21 Changed 9 years ago by jordan

The plan for this ticket is that after libevent starts bundling evhtp, we'll use evhtp instead of libcurl and add speed limit support for webseed downloads.\

Changed 9 years ago by vipjml

use this "web.c" can fix this problen, this web.c is modified based on 2.77

comment:22 Changed 9 years ago by vipjml

the change to web.c was very little.

I return CURL_WRITEFUNC_PAUSE in writeFunc to stop curl's downloading when there is no bandwith,and add the curl handle to a list for restart downloading later.

THREADFUNC_MAX_SLEEP_MSEC wat reduce to achieve more accurate band limit.

it's important to know that curl_easy_pause will cause a call to writeFunc

comment:23 follow-up: Changed 9 years ago by jordan

Thanks vipjml! This is a good idea. I wasn't aware of CURL_WRITEFUNC_PAUSE.

comment:24 Changed 9 years ago by jordan

  • Resolution set to fixed
  • Status changed from reopened to closed

I've committed a version of the patch to r14071. What I changed was to add a torrent id to the web task struct s.t. we could clamp bandwidth on per-torrent speed limits as well as the per-session speed limit.

comment:25 Changed 9 years ago by Robby

Should this ticket get a version milestone?

comment:26 Changed 9 years ago by jordan

  • Milestone changed from Sometime to 2.80

Indeed.

comment:27 in reply to: ↑ 23 Changed 9 years ago by vipjml

Replying to jordan:

Thanks vipjml! This is a good idea. I wasn't aware of CURL_WRITEFUNC_PAUSE.

while ((handle = tr_list_pop_front (&paused_easy_handles)))

curl_easy_pause (handle, CURLPAUSE_CONT);

I think this while loop maybe cause problem. curl_easy_pause will trigger a call to writeFunc which will problem add a handle to paused_easy_handles,so this line will loop until there are free bandwith, a lots of cpus will be used.

I used to encounter the problem, my curl version is 7.24.

comment:28 follow-up: Changed 8 years ago by jordan

vipjml, I think r14074 should resolve this and it's behaving correctly for me. Do you have an opinion on this revision?

comment:29 in reply to: ↑ 28 Changed 8 years ago by vipjml

Replying to jordan:

vipjml, I think r14074 should resolve this and it's behaving correctly for me. Do you have an opinion on this revision?

I think so.

comment:30 Changed 8 years ago by alexat

I wonder if it's a good idea to use tr_bandwidthclamp() in the web thread without using some kind of thread synchronization. I think that's why I didn't do it that way but I'm not sure anymore.

Is there a reason why my code has never been implemented btw?

Note: See TracTickets for help on using tickets.