From a96c1d5d9a8403b49a75b7faf3aa16d6d31abfdd Mon Sep 17 00:00:00 2001 From: Matt Joiner Date: Wed, 6 Aug 2025 20:34:08 +1000 Subject: [PATCH] Skip download rate limited reader wrapping if DownloadRateLimiter is nil --- client-peerconn_test.go | 5 ++--- client.go | 18 +++++++++++++----- config.go | 21 ++++++++++++++------- peerconn_test.go | 4 +--- rate.go | 24 +++++++++++++++++++----- ratelimitreader.go | 11 +++++++++++ torrent.go | 7 ++----- 7 files changed, 62 insertions(+), 28 deletions(-) diff --git a/client-peerconn_test.go b/client-peerconn_test.go index fa1cff08..372b4af4 100644 --- a/client-peerconn_test.go +++ b/client-peerconn_test.go @@ -1,6 +1,7 @@ package torrent import ( + "cmp" "io" "os" "testing" @@ -90,9 +91,7 @@ func testClientTransfer(t *testing.T, ps testClientTransferParams) { cfg.Seed = true // Some test instances don't like this being on, even when there's no cache involved. cfg.DropMutuallyCompletePeers = false - if ps.SeederUploadRateLimiter != nil { - cfg.UploadRateLimiter = ps.SeederUploadRateLimiter - } + cfg.UploadRateLimiter = cmp.Or(ps.SeederUploadRateLimiter, cfg.UploadRateLimiter) cfg.DataDir = greetingTempDir if ps.ConfigureSeeder.Config != nil { ps.ConfigureSeeder.Config(cfg) diff --git a/client.go b/client.go index 473aa871..fd3466e2 100644 --- a/client.go +++ b/client.go @@ -1740,10 +1740,7 @@ func (cl *Client) newConnection(nc net.Conn, opts newConnectionOpts) (c *PeerCon c.peerImpl = c c.setPeerLoggers(cl.logger, cl.slogger) c.setRW(connStatsReadWriter{nc, c}) - c.r = &rateLimitedReader{ - l: cl.config.DownloadRateLimiter, - r: c.r, - } + c.r = cl.newDownloadRateLimitedReader(c.r) c.logger.Levelf( log.Debug, "inited with remoteAddr %v network %v outgoing %t", @@ -1755,6 +1752,17 @@ func (cl *Client) newConnection(nc net.Conn, opts newConnectionOpts) (c *PeerCon return } +func (cl *Client) newDownloadRateLimitedReader(r io.Reader) io.Reader { + if cl.config.DownloadRateLimiter == nil { + return r + } + // Why if the limit is Inf? Because it can be dynamically adjusted. + return rateLimitedReader{ + l: cl.config.DownloadRateLimiter, + r: r, + } +} + func (cl *Client) onDHTAnnouncePeer(ih metainfo.Hash, ip net.IP, port int, portOk bool) { cl.lock() defer cl.unlock() @@ -1943,7 +1951,7 @@ func (cl *Client) underWebSeedHttpRequestLimit(key webseedHostKeyHandle) bool { // Check for bad arrangements. This is a candidate for an error state check method. func (cl *Client) checkConfig() error { - if cl.config.DownloadRateLimiter.Limit() == 0 { + if EffectiveDownloadRateLimit(cl.config.DownloadRateLimiter) == 0 { if len(cl.dialers) != 0 { return errors.New("download rate limit is zero, but dialers are set") } diff --git a/config.go b/config.go index a0b1e082..02eb61bf 100644 --- a/config.go +++ b/config.go @@ -79,6 +79,8 @@ type ClientConfig struct { // rate-limiting io.Reader minus one. This is likely to be the larger of the main read loop // buffer (~4096), and the requested chunk size (~16KiB, see TorrentSpec.ChunkSize). If limit is // not Inf, and burst is left at 0, the implementation will choose a suitable burst. + // + // If the field is nil, no rate limiting is applied. And it can't be adjusted dynamically. DownloadRateLimiter *rate.Limiter // Maximum unverified bytes across all torrents. Not used if zero. MaxUnverifiedBytes int64 @@ -233,7 +235,6 @@ func NewDefaultClientConfig() *ClientConfig { MaxAllocPeerRequestDataPerConn: 1 << 20, ListenHost: func(string) string { return "" }, UploadRateLimiter: unlimited, - DownloadRateLimiter: unlimited, DisableAcceptRateLimiting: true, DropMutuallyCompletePeers: true, HeaderObfuscationPolicy: HeaderObfuscationPolicy{ @@ -264,10 +265,16 @@ type HeaderObfuscationPolicy struct { func (cfg *ClientConfig) setRateLimiterBursts() { // What about chunk size? - setRateLimiterBurstIfZero(cfg.UploadRateLimiter, cfg.MaxAllocPeerRequestDataPerConn) - setRateLimiterBurstIfZero( - cfg.DownloadRateLimiter, - min( - int(cfg.DownloadRateLimiter.Limit()), - defaultDownloadRateLimiterBurst)) + if cfg.UploadRateLimiter.Burst() == 0 { + cfg.UploadRateLimiter.SetBurst(cfg.MaxAllocPeerRequestDataPerConn) + } + setDefaultDownloadRateLimiterBurstIfZero(cfg.DownloadRateLimiter) +} + +// Returns the download rate.Limit handling the special nil case. +func EffectiveDownloadRateLimit(l *rate.Limiter) rate.Limit { + if l == nil { + return rate.Inf + } + return l.Limit() } diff --git a/peerconn_test.go b/peerconn_test.go index 5cceb9c8..876ed883 100644 --- a/peerconn_test.go +++ b/peerconn_test.go @@ -90,9 +90,7 @@ func (me *torrentStorage) WriteAt(b []byte, _ int64) (int, error) { func BenchmarkConnectionMainReadLoop(b *testing.B) { var cl Client - cl.init(&ClientConfig{ - DownloadRateLimiter: unlimited, - }) + cl.init(&ClientConfig{}) cl.initLogger() ts := &torrentStorage{} t := cl.newTorrentForTesting() diff --git a/rate.go b/rate.go index b711cc91..d81eda20 100644 --- a/rate.go +++ b/rate.go @@ -1,18 +1,32 @@ package torrent import ( + "math" + "golang.org/x/time/rate" ) -// 64 KiB used to be a rough default buffer for sockets on Windows. I'm sure it's bigger -// these days. What about the read buffer size mentioned elsewhere? Note this is also used for -// webseeding since that shares the download rate limiter by default. -const defaultDownloadRateLimiterBurst = 1 << 16 +// 64 KiB used to be a rough default buffer for sockets on Windows. I'm sure it's bigger these days. +// What about the read buffer size mentioned elsewhere? Note this is also used for webseeding since +// that shares the download rate limiter by default. 1 MiB is the default max read frame size for +// HTTP/2, +const defaultMinDownloadRateLimiterBurst = 1 << 20 // Sets rate limiter burst if it's set to zero which is used to request the default by our API. func setRateLimiterBurstIfZero(l *rate.Limiter, def int) { - if l.Burst() == 0 && l.Limit() != rate.Inf { + // Set it to something reasonable if the limit is Inf, in case the limit is dynamically adjusted + // and the user doesn't know what value to use. Assume the original limit is in a reasonable + // ballpark. + if l != nil && l.Burst() == 0 { // What if the limit is greater than what can be represented by int? l.SetBurst(def) } } + +// Sets rate limiter burst if it's set to zero which is used to request the default by our API. +func setDefaultDownloadRateLimiterBurstIfZero(l *rate.Limiter) { + setRateLimiterBurstIfZero(l, int( + max( + min(EffectiveDownloadRateLimit(l), math.MaxInt), + defaultMinDownloadRateLimiterBurst))) +} diff --git a/ratelimitreader.go b/ratelimitreader.go index 63a56176..1bd743ed 100644 --- a/ratelimitreader.go +++ b/ratelimitreader.go @@ -8,6 +8,17 @@ import ( "golang.org/x/time/rate" ) +func newRateLimitedReader(r io.Reader, l *rate.Limiter) io.Reader { + if l == nil { + // Avoids taking Limiter lock to check limit, and allows type assertions to bypass Read. + return r + } + return rateLimitedReader{ + l: l, + r: r, + } +} + type rateLimitedReader struct { l *rate.Limiter r io.Reader diff --git a/torrent.go b/torrent.go index f8a50b92..5f6c75d0 100644 --- a/torrent.go +++ b/torrent.go @@ -3110,12 +3110,9 @@ func (t *Torrent) addWebSeed(url string, opts ...AddWebSeedsOpt) bool { for _, opt := range opts { opt(&ws.client) } - setRateLimiterBurstIfZero(ws.client.ResponseBodyRateLimiter, defaultDownloadRateLimiterBurst) + setDefaultDownloadRateLimiterBurstIfZero(ws.client.ResponseBodyRateLimiter) ws.client.ResponseBodyWrapper = func(r io.Reader) io.Reader { - return &rateLimitedReader{ - l: ws.client.ResponseBodyRateLimiter, - r: r, - } + return newRateLimitedReader(r, ws.client.ResponseBodyRateLimiter) } g.MakeMapWithCap(&ws.activeRequests, ws.client.MaxRequests) ws.locker = t.cl.locker() -- 2.51.0