echo icon indicating copy to clipboard operation
echo copied to clipboard

RateLimiter rejects all events when Limit is between 0 and 1

Open aschiffmann opened this issue 3 years ago • 6 comments

Checklist

  • [x] Dependencies installed
  • [x] No typos
  • [x] Searched existing issues and docs

Issue Description

When using the RateLimiter Middleware with a rate between 0 and 1 all events will be rejected instead of applying the specified rate. E.g.: e.Use(middleware.RateLimiter(middleware.NewRateLimiterMemoryStore(0.5)))

I am not saying that it is a common use case to have a rate.Limit between 0 and 1, but it is very confusing that NewRateLimiterMemoryStore() accepts a value of type float, the docs don't mention any restrictions for non-zero values but still the argument is interpreted as zero instead of its actual value.

Expected behaviour

One of those:

  • the docs explains how a fractional rate.Limit is handled
  • NewRateLimiterMemoryStore() only accepts integer values
  • the RateLimiter interprets a fractional rate.Limit correctly, e.g. a limit of 0.5 will result in an equivalent rate of 30 requests/minute

Actual behaviour

a fractional rate.Limit between 0 and 1 is surprisingly interpreted like a zero Limit

Version/commit

v4.2.2

aschiffmann avatar Apr 21 '21 14:04 aschiffmann

As ratelimiter middleware (default implementation) is wrapper around https://pkg.go.dev/golang.org/x/time/rate library and rate value goes directly to that library I think the docs explains how a fractional rate.Limit is handled would be correct solution.

aldas avatar Apr 27 '21 07:04 aldas

NewRateLimiterMemoryStore documentation states example like that

Example (with 20 requests/sec):

limiterStore := middleware.NewRateLimiterMemoryStore(20)

So if we pass float value like 0.5 in NewRateLimiterMemoryStore it should limit amount request per second or per minute?

@aschiffmann says per minute but in the docs we have second

LukaJaj avatar Aug 31 '21 11:08 LukaJaj

This is actual documentation https://pkg.go.dev/golang.org/x/time/rate#Limit

aldas avatar Aug 31 '21 11:08 aldas

From listed solutions (

  • the docs explains how a fractional rate.Limit is handled
  • NewRateLimiterMemoryStore() only accepts integer values
  • the RateLimiter interprets a fractional rate.Limit correctly, e.g. a limit of 0.5 will result in an equivalent rate of 30 requests/minute

) which one should I work on ?

LukaJaj avatar Nov 21 '21 06:11 LukaJaj

Changing from rate.Limit to int* would probably break others existing code.

As this middleware just wraps x/time/rate code it would be best to add comments inside middleware where we have rate.Limit about 0-1 behaviour and link to actual limiter library docs. And also note of that behavior in https://github.com/labstack/echox/blob/master/website/content/middleware/rate-limiter.md

aldas avatar Nov 21 '21 14:11 aldas

So if we pass float value like 0.5 in NewRateLimiterMemoryStore it should limit amount request per second or per minute?

@aschiffmann says per minute but in the docs we have second

You are right, the rate is "per second" and not "per minute". Sorry for the confusion

aschiffmann avatar Dec 06 '21 15:12 aschiffmann