search

antifuchs / ratelimit_futures

Rate-limiting for rust futures v0.1
MIT License
5 stars 2 forks source link

Lifetime issues #2

Open Johni0702 opened 5 years ago

Johni0702 commented 5 years ago

Is there any particular reason why the limiter is passed to Ratelimit via a mutable reference rather than being moved? It seems strange considering one can clone DirectRateLimiter however much one wants.

More importantly though, afaict it requires you to keep the DirectRateLimiter alive for at least as long as the Future is alive, which (on futures 0.1) quickly gets problematic once you need a future with 'static lifetime, e.g. to spawn it.

If there's no particular reason, would you be willing to accept a PR which changes that? Also, while I'm at it, would you be willing to accept a PR adding a Stream::ratelimit combinator or should I rather put that in a separate crate of my own?

antifuchs commented 5 years ago

You're right, this could be better, and some of this is because I'm potentially a bit confused about how to handle Arcs and façades. I wrote my thinking of a few months ago up in https://github.com/antifuchs/ratelimit_meter/issues/23, which is probably not right anymore, now that there's futures bindings.

The need to .clone in-memory rate limiters really is confusing, but what really happens is that you end up cloning the Arc containing the mutexed rate limiter state, which is kept unique. I would love to document this better.

All that said, I'd be thrilled to take PRs to fix lifetime issues, especially those that keep you from getting work done! (I only wrote this as a proof-of-concept at first, and am not actively using the library at the moment.)

Also would love a PR to extend this for Streams!

I'll leave this issue open for now, to remind me to sync up with https://github.com/antifuchs/ratelimit_meter/issues/23 when the docs get better (: