Skip to content
This repository has been archived by the owner on Aug 24, 2019. It is now read-only.

Multiple limiters #25

Merged
merged 8 commits into from
Oct 28, 2016
Merged

Multiple limiters #25

merged 8 commits into from
Oct 28, 2016

Conversation

soffes
Copy link
Owner

@soffes soffes commented Oct 20, 2016

Instead of a single, global timed rate limiter, make single limiters per thing. This will be version 2.0.0.

Upgrading

Here's a comparison of the current API and the new API:

// Current
RateLimit.execute(name: "timeline", limit: 5) {
    refresh()
}

// Proposed
let timeline = TimedLimiter(limit: 5)
timeline.execute {
    refresh()
}

I'm also adding a wrapper for backwards compatibility that will be in there until 2.1.0.

New Limiters

In addition to the old style timed limiter, new limiters will be much easier due to the new Limiter protocol:

public protocol Limiter {
    func execute(_ block: () -> Void)
    func reset()
}

Here's one for saying something can run a set number of times:

let tutorialPopOver = CountedLimiter(limit: 3)
tutorialPopOver.execute {
    showSomeBigAnnoyingAnimation()
}

I'd also love to make one that debounces. This would be especially useful for stuff like search as you type and other applications.

@soffes soffes added the wip label Oct 20, 2016
@soffes
Copy link
Owner Author

soffes commented Oct 20, 2016

This addresses #10 and #20

@soffes
Copy link
Owner Author

soffes commented Oct 20, 2016

@hyperspacemark @calebd @eliperkins @ayanonagon @subdigital @AnthonyMDev I'd love some thoughts on this approach if you have a moment. ❤️

@subdigital
Copy link

2 things:

  • I like this a lot better than using the string name. Especially for invalidating a limiter for cases where you know you want the work done
  • you could still support the old API by having a dictionary of [String:TimedLimited] and just use the new API internally

@calebd
Copy link
Contributor

calebd commented Oct 20, 2016

It's funny you bring this up because I was thinking of proposing this exact approach a few days ago. This looks great!

let now = Date()
private static let queue = DispatchQueue(label: "com.samsoffes.ratelimit", attributes: [])

private static var dictionary = [String: TimedLimiter]()
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@subdigital you mean like this? 😛

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha! Yeah 😉

@soffes
Copy link
Owner Author

soffes commented Oct 20, 2016

@subdigital cool. The only thing with name is it may be more annoying to keep track of these things. Would that be annoying in your uses?

@subdigital
Copy link

I’m thinking you could use a name if you wanted to, but maybe it defaults to a UUID string in cases where you don’t care.

For instance, it avoids needing a constant string for simple cases like this:

        let rateLimitName = "save"

        if progress == 1.0 {
            RateLimit.resetLimitForName(rateLimitName)
        }

        RateLimit.execute(name: rateLimitName, limit: 0.5) {
            try? PersistenceManager.save(context: context)
        }

@subdigital
Copy link

This would end up w/ a private variable instead:

private progressSaveLimiter = TimedLimiter(limit: 0.5)

...

func onProgress(notification: NSNotification) {
  if progress == 1.0 { 
    progressRateLimiter.reset()
  }

  progressRateLimiter.execute() { ... }
}

@calebd
Copy link
Contributor

calebd commented Oct 20, 2016

I would prefer to get rid of the named API entirely. A consumer can take care of scope themselves by declaring a global (probably private to a given file) rate limit that is used. The use case is much more clear and you can't accidentally leak that detail to another place in your app by reusing a string name.

@eliperkins
Copy link

This API looks awesome! I'm 👍 on it.

The only gotcha I can see is having to change things to retain TimedLimiter objects to satiate object lifetimes, where as the old API was pretty YOLO with that since we were using the static object to record things.

I'm all for this!

@soffes
Copy link
Owner Author

soffes commented Oct 20, 2016

@eliperkins ya I'm afraid that would be annoying. That said I'm all about less global state.

@calebd that's a great idea. The only issue is something like sign out where you want to reset all limiters.

@subdigital ya, I'm not sure. Now there's a bunch of limiters, that might be awkward. I guess each one could take a global set of them, but that seems messy. Consumers could do this. I'm just worried it might be too much work though.

You could get back the old global style by simply making a bunch of global variables:

let timeline = TimedLimiter(limit: 1)
let activity = TimedLimiter(limit: 1)

func resetAll() {
    [timeline, activity].forEach(reset)
}

Then you could do the same thing as you did before and now it's safe with variable names instead of strings :)

I think I'm warming up to it. What do you all think?

@subdigital
Copy link

In my experience I don't tend to have tons of these things. I've only ever used 1 or 2 in a given app.

@soffes
Copy link
Owner Author

soffes commented Oct 20, 2016

I have a first draft at debouncing one (#10).

I made a new protocol called AsyncLimiter. I was thinking for these you'd have to initialize it with a block and then that's called whenever it's ready. I'd love some eyes on this. I feel like this would be perfect for typing while searching, etc. I'm thinking maybe I need to have an option to run right away if there's not already one queued.

@calebd
Copy link
Contributor

calebd commented Oct 20, 2016

Most of my uses for this are scoped to a view controller so those would naturally fall out of scope when the user signs out. Cases that don't fall under that could be achieved with notifications and a reset function.

@soffes
Copy link
Owner Author

soffes commented Oct 21, 2016

Does anyone use the persistent version of this?

@soffes
Copy link
Owner Author

soffes commented Oct 21, 2016

I was thinking I could possibly make all of these conform to NSCoding so you could use them with state restoration or whatever else easily and just do your own.

@subdigital
Copy link

I’m thinking you could use a name if you wanted to, but maybe it defaults to a UUID string in cases where you don’t care.

For instance, it avoids needing a constant string for simple cases like this:

        let rateLimitName = "save"

        if progress == 1.0 {
            RateLimit.resetLimitForName(rateLimitName)
        }

        RateLimit.execute(name: rateLimitName, limit: 0.5) {
            print("Saving progress... \(progress)")
            try? PersistenceManager.save(context: context)
        }

On Oct 20, 2016, at 1:58 PM, Sam Soffes notifications@github.com wrote:

@subdigital https://github.com/subdigital cool. The only thing with name is it may be more annoying to keep track of these things. Would that be annoying in your uses?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #25 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AADnBDR1D1MAB0ZTOIWJC60jMQfp7AvWks5q17nlgaJpZM4KceTH.

@soffes soffes removed the wip label Oct 28, 2016
syncQueue.sync {
if count < limit {
count += 1
block()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it clear from the outside that block is executed on a different queue? I feel like this could have unintended side effects. I know that syncing to a queue sometimes means you operate on the same thread but afaik that behavior is not guaranteed by GCD.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right. That's bad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really figure out how to work around that though. I guess you could sync only around the counter stuff and call the block right after if you determined that it can be run.

//

public protocol SyncLimiter {
@discardableResult func execute(_ block: () -> Void) -> Bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love that you get a value back!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the value of if it ran or not. Not the value of the block like DispatchQueue.sync.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Though a @discardableResult func execute<T>(_ block: () -> T) -> T could be cool. 😜

@soffes soffes merged commit d6efa68 into master Oct 28, 2016
@soffes soffes deleted the multi branch October 28, 2016 22:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants