vutran1710 / PyrateLimiter

⚔️Python Rate-Limiter using Leaky-Bucket Algorithm Family
https://pyratelimiter.readthedocs.io
MIT License
334 stars 36 forks source link

fix: async bucket_factory.get() #171

Closed haoyuhu closed 4 months ago

haoyuhu commented 4 months ago

Change-Id: Ib471e7661c7d9808ddad3767806b08f1ef1b9967

I have discovered an error that occurs at runtime when I use aioredis's asyncio redis_client to build the RedisBucket. The error message is "Invalid bucket: item: {name}". I have attempted to fix this bug and would appreciate your assistance with the code review, thank you very much!

class ToolRuntimeMultiBucketFactory(BucketFactory):
    def __init__(self, clock: AbstractClock = TimeClock()):
        self.clock = clock
        self.buckets = {}
        self.thread_pool = None

    def wrap_item(self, runtime: ToolRuntimeParams, weight: int = 1) -> RateItem:
        """Time-stamping item, return a RateItem"""
        now = self.clock.now()
        return ToolRuntimeRateItem(runtime, now, weight=weight)

    async def get(self, item: ToolRuntimeRateItem) -> AbstractBucket:
        rates = _build_rates(item.runtime.quota_obj())
        if item.name not in self.buckets:
            # Use `self.create(..)` method to both initialize new bucket and calling `schedule_leak` on that bucket
            # We can create different buckets with different types/classes here as well
            new_bucket = await RedisBucket.init(rates, async_redis_client, f"rl_bucket:{item.name}")
            self.schedule_leak(new_bucket, self.clock)
            self.buckets.update({item.name: new_bucket})

        bucket: AbstractBucket = self.buckets[item.name]
        bucket.rates = rates
        return bucket
vutran1710 commented 4 months ago

the code basically looks good, but I think you will have to add some test for this async-get-bucket method.

You can modify the bucket-factory test to cover the case where get-bucket is async.

vutran1710 commented 4 months ago

And use pre-commit to fix the lint complaints as well

vutran1710 commented 4 months ago

Somehow the test suite did not pass :(. Can you check that? @haoyuhu

haoyuhu commented 4 months ago

Somehow the test suite did not pass :(. Can you check that? @haoyuhu

I have tried to fix the aforementioned issue, but the unit tests are not running properly on my local machine. Additionally, regarding the error message "Avoid too many return statements within this function", I don't have a better idea at the moment. Do you have any suggestions or recommendations?

vutran1710 commented 4 months ago

Somehow the test suite did not pass :(. Can you check that? @haoyuhu

I have tried to fix the aforementioned issue, but the unit tests are not running properly on my local machine. Additionally, regarding the error message "Avoid too many return statements within this function", I don't have a better idea at the moment. Do you have any suggestions or recommendations?

You can ignore the "too many return" thing.

But if the tests are not running well then its probably due to pytest-asyncio & nox. Im afraid you have to play with it a bit to get it right.

haoyuhu commented 4 months ago

I have fixed the issues with the unit tests, and I have also gone through the code and gained a deeper understanding. Please help me confirm if there are any areas that have been overlooked. Thank you!

vutran1710 commented 4 months ago

Everything looks good.

haoyuhu commented 4 months ago

Everything looks good.

Can you help me create a new version? I'm looking forward to being able to download the latest version from PyPI. My code depends on this fix. Thank you.

vutran1710 commented 4 months ago

Everything looks good.

Can you help me create a new version? I'm looking forward to being able to download the latest version from PyPI. My code depends on this fix. Thank you.

It's done!