Update the Limiter interface to accept a context.Context object #11

achals · 2017-10-09T21:48:24Z

This also removes the Clock abstraction, since there doesn't seem to be a way to create timers using the mock clock effectively, and it isn't used in yab at all. I can add it back in and try to munge it to work if it has value.

…move the Clock abstraction

CLAassistant · 2017-10-09T21:48:31Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

achals · 2017-10-09T21:48:41Z

Addresses #10

achals · 2017-10-09T21:51:23Z

cc @kriskowal @prashantv

prashantv

Overall, I like the direction of this change, but we also haven't motivated the reasoning for some of these changes

The interface is now a blocking interface (similar to yab) rather than letting the caller do the sleep. It makes sense, but I'd like some motivation for the change so we don't just flip-flop between the 2 interfaces
Since we don't use a mock clock, tests now rely on real time (both within this package, and external packages using it). We need a way of testing that doesn't end up sleeping for some number of real seconds. Tests that rely on real time tend to be flaky, or end up taking a long time.

prashantv · 2017-10-23T16:42:20Z

ratelimit.go

@@ -24,7 +24,8 @@ import (
 	"sync"
 	"time"

-	"go.uber.org/ratelimit/internal/clock"
+	"context"


nit: no blank between stdlib imports

still see a blank between "time" and "context"

prashantv · 2017-10-23T16:44:05Z

ratelimit.go

@@ -63,24 +56,14 @@ func New(rate int, opts ...Option) Limiter {
 	l := &limiter{
 		perRequest: time.Second / time.Duration(rate),
 		maxSlack:   -10 * time.Second / time.Duration(rate),
+		timer:      time.NewTimer(time.Duration(math.MaxInt64)),


rather than use a max duration, should we create a timer, stop it (maybe in init) and use that everywhere?

I can also leave the timer field as nil within the struct and have a check for it in Take & set it when needed - but I'm not sure if there are any performance implications.

nil check works too, but less branches is always nice, so might be better to have an expired timer.

prashantv · 2017-10-23T16:44:24Z

ratelimit.go

-type Clock interface {
-	Now() time.Time
-	Sleep(time.Duration)
+	Take(context.Context) bool


what does the bool argument mean?

Right now, Take returns a True if it was unblocked without any cancellations from the passed in context, and False if it was unblocked because of the context.

Can we update the documentation to make that part of the API

I'm also not sure how valuable it is, since the caller can just check ctx.Err() != nil

What is the value in having this in the API?

prashantv · 2017-10-23T16:46:29Z

ratelimit.go

@@ -118,14 +102,19 @@ func (t *limiter) Take() time.Time {

 	// If sleepFor is positive, then we should sleep now.
 	if t.sleepFor > 0 {
-		t.clock.Sleep(t.sleepFor)
+		t.timer.Stop()
+		t.timer.Reset(t.sleepFor)


stopping and resetting a timer is racy, take a look at the documentation for how to stop and reset:
https://golang.org/pkg/time/#Timer.Reset

if !t.Stop() { <-t.C } t.Reset(d)

Good catch, thanks!

achals · 2017-10-24T05:15:47Z

@prashantv Thanks a lot for the feedback! Within Hailstorm, and also generally speaking, it seems like the Limiter interface is meant to be used explicitly in the case when some operation is meant to be repeated and maintain a specific RPS - i.e. there would probably need to be blocking in the most common use cases.

In that case, having the goroutine block as a result of the call to the Limiter seems like the most complete experience, from a users' perspective, and frees me from having to rebuild blocking code which might be error prone.

For testing, I'm certainly open to adding back the mock clock since I like the abstraction as well - lemme see if there's a way of accomplishing that.

achals · 2017-10-24T22:00:32Z

Also @prashantv , correct me if I'm wrong but the old interface still blocking, is it not? The docs as well as the implementation state that the Limiter is meant to sleep till the rps is met.

prashantv

So this package originally was a copy of yab's rate limiter, which was blocking. However, it was then modified to be non-blocking. This is why I wanted to make sure we had good motivation for making it blocking again.

For example, we have use cases where we don't want to sleep, but want to deny the request if it exceeds the rate limit. Ideally the same code could be used for both.

prashantv · 2017-11-01T00:04:19Z

ratelimit.go

@@ -24,7 +24,8 @@ import (
 	"sync"
 	"time"

-	"go.uber.org/ratelimit/internal/clock"
+	"context"


still see a blank between "time" and "context"

prashantv · 2017-11-01T00:05:04Z

ratelimit.go

-type Clock interface {
-	Now() time.Time
-	Sleep(time.Duration)
+	Take(context.Context) bool


Can we update the documentation to make that part of the API

I'm also not sure how valuable it is, since the caller can just check ctx.Err() != nil

What is the value in having this in the API?

prashantv · 2017-11-01T00:05:45Z

ratelimit.go

@@ -63,24 +56,14 @@ func New(rate int, opts ...Option) Limiter {
 	l := &limiter{
 		perRequest: time.Second / time.Duration(rate),
 		maxSlack:   -10 * time.Second / time.Duration(rate),
+		timer:      time.NewTimer(time.Duration(math.MaxInt64)),


nil check works too, but less branches is always nice, so might be better to have an expired timer.

jeromefroe · 2018-09-26T01:09:20Z

Is there any interest in reviving this PR? I have a use case where I would like to be able to pass the Limiter a Context so that it can return early if the Context is cancelled. Currently, the code looks like the following:

type Thing struct {
    limiter ratelimit.Limiter
}

func (t *Thing) Do(ctx context.Context) error {
    t.limiter.Take()

    select {
    case <-ctx.Done():
         return ctx.Err()
    default:
    }

    // expensive work ...
}

The code works as is, but it would be nice, and more efficient, if I could combine the two steps into one:

func (t *Thing) Do(ctx context.Context) error {
    if _, err := <-t.limiter.TakeWithCtx(ctx); err != nil {
        return err
     }

    // expensive work ...
}

Implementation-wise it should, in theory, be relatively trivial to support such a method. The heart of TakeWithCtx could look like the following:

func (t *limiter) TakeWithCtx() (time.Time, error) {
    // setup work to determine the amount of time we should sleep for ...

    timer := time.NewTimer(t.sleepFor)
    select {
    case <-ctx.Done():
        // possible cleanup work ...

        timer.Stop()
        return time.Time{}, ctx.Error()
    case <-timer.C:
    }

    // ...
}

In practice I imagine this change might be more difficult though because:

It requires a new method, TakeWithCtx, on the Limiter interface which will be a breaking change.
It requires the Clock interface to be re-done as TakeWithCtx won't work if the Sleep method is blocking, another breaking change.

I'd be curious to know if anyone else see values in such a change?

prashantv · 2018-09-26T18:32:13Z

This repo has no releases so breaking changes are fine. I don't think there's really an owner right now -- this is a copy of the rate limiting code in yab which I wrote for limiting the number of requests sent by yab. However, this library isn't used by yab, we can iterate on the APIs to find something that makes sense for everyone.

achals · 2018-09-26T21:56:29Z

I'm up for reviving this PR, if it will prove useful. I'll spend some time time addressing @prashantv's comments from last time and update.

jeromefroe · 2018-09-27T00:21:05Z

Thanks @achals, that would be great! I looked into this a little yesterday and figured I would share some thoughts I had in case it would be helpful.

The documentation for the Clock interface states:

Clock is the minimum necessary interface to instantiate a rate limiter with a clock or mock clock, compatible with clocks created using github.com/andres-erbsen/clock.

I took a look at the github.com/andres-erbsen/clock package and the interface for Clock is:

type Clock interface {
    Now() time.Time
    Sleep(d time.Duration)
    Timer(d time.Duration) *Timer
    // other methods ...
}

So we could add a Timer(d time.Duration) *Timer method to the Clock interface in the package and still be compatible with the Clock interface in the github.com/andres-erbsen/clock package (though we would have to reuse the Timer struct from ithub.com/andres-erbsen/clock).

TakeWithCtx could then be exactly the same as Take except we could replace the call to t.clock.Sleep(t.sleepFor) with:

var (
    err error
    timer = t.clock.Timer(t.sleepFor)
)

select {
case <-ctx.Done():
    err = ctx.Error()
case <-timer.C:

if err != nil {
    // ...
}

Achal Shah added 3 commits October 4, 2017 22:49

Update the interfacae for the Limiter interfacae

fa470e2

Update the ratelimit interface to accept a context.Context object, re…

593774b

…move the Clock abstraction

Remove artifical test delay

83d8d3e

prashantv suggested changes Oct 23, 2017

View reviewed changes

Deal with race condition for timer.Stop

12880b7

prashantv reviewed Nov 1, 2017

View reviewed changes

achals closed this Jun 26, 2020

rabbbit mentioned this pull request Jul 11, 2021

context.Context support for Take? #73

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the Limiter interface to accept a context.Context object #11

Update the Limiter interface to accept a context.Context object #11

achals commented Oct 9, 2017

CLAassistant commented Oct 9, 2017 •

edited

Loading

achals commented Oct 9, 2017

achals commented Oct 9, 2017

prashantv left a comment

prashantv Oct 23, 2017

achals Oct 24, 2017

prashantv Nov 1, 2017

prashantv Oct 23, 2017

achals Oct 24, 2017

prashantv Nov 1, 2017

prashantv Oct 23, 2017

achals Oct 24, 2017

prashantv Nov 1, 2017

prashantv Oct 23, 2017

achals Oct 24, 2017

achals commented Oct 24, 2017

achals commented Oct 24, 2017

prashantv left a comment

prashantv Nov 1, 2017

prashantv Nov 1, 2017

prashantv Nov 1, 2017

jeromefroe commented Sep 26, 2018

prashantv commented Sep 26, 2018

achals commented Sep 26, 2018

jeromefroe commented Sep 27, 2018

Update the Limiter interface to accept a context.Context object #11

Update the Limiter interface to accept a context.Context object #11

Conversation

achals commented Oct 9, 2017

CLAassistant commented Oct 9, 2017 • edited Loading

achals commented Oct 9, 2017

achals commented Oct 9, 2017

prashantv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achals commented Oct 24, 2017

achals commented Oct 24, 2017

prashantv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromefroe commented Sep 26, 2018

prashantv commented Sep 26, 2018

achals commented Sep 26, 2018

jeromefroe commented Sep 27, 2018

CLAassistant commented Oct 9, 2017 •

edited

Loading