Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle 'None' return value of wait() properly during throttling. #6837

Merged
merged 4 commits into from Aug 12, 2019

Conversation

EnTeQuAk
Copy link
Contributor

Fixes #6836

@rpkilby
Copy link
Member

rpkilby commented Jul 26, 2019

Thanks @EnTeQuAk. Adding to the milestone.

@rpkilby rpkilby added this to the 3.10.1 Release milestone Jul 26, 2019
@tomchristie
Copy link
Member

Okay, just noticed this after rolling 3.10.2.

Looks like this, #6833, #6834, and #6844 might be good candidates for another minor release later this week.

rest_framework/views.py Outdated Show resolved Hide resolved
tests/test_throttling.py Outdated Show resolved Hide resolved
rest_framework/views.py Outdated Show resolved Hide resolved
rest_framework/views.py Outdated Show resolved Hide resolved
tests/test_throttling.py Outdated Show resolved Hide resolved
@rpkilby
Copy link
Member

rpkilby commented Aug 7, 2019

Thanks @EnTeQuAk!

Copy link
Member

@rpkilby rpkilby left a comment

Choose a reason for hiding this comment

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

Actually, on second thought, what's the correct behavior here? If throttle_durations = [None], this would still end up calling self.throttled(request, None), which doesn't seem right? Maybe the fix should be to .allow_request(). It doesn't make sense for allow_request to return False for the throttle to then return a None wait value.

@kevin-brown
Copy link
Member

@rpkilby I believe this PR fixes the issue where max([None, 10, 15]) triggers an exception while we expected it to return 15 to align with always returning the highest throttle amount.

If all throttles return None, then it that means one of them requested a throttle and none of them actually specified a value, in which case we should throttle with None like we used to.

@rpkilby
Copy link
Member

rpkilby commented Aug 7, 2019

I guess I'm asking if this makes sense:

then it that means one of them requested a throttle and none of them actually specified a value

Does it make sense for a throttle to say "hey, this request should be throttled", and then when we ask how long it should wait for it responds with ¯\_(ツ)_/¯

For context, I'm not incredibly familiar with the throttling internals, which is why I'm asking.

@diox
Copy link
Contributor

diox commented Aug 12, 2019

(I wrote the patch causing this issue)

Does it make sense for a throttle to say "hey, this request should be throttled", and then when we ask how long it should wait for it responds with ¯_(ツ)_/¯

Yes. You might not want to tell clients how long they should wait. Or you might not even know the exact answer (it's a guess anyway, a recommendation, nothing more), depending on the implementation. BaseThrottle.wait() default implementation is to return None, and is documented as optional, so it makes sense to support that.

What threw me off when I wrote the initial patch was that, even though I had this in mind, I forgot that you could mix throttles that do return a duration and ones that don't (completely glossed over the fact that SimpleRateThrottle.wait() could return None if it computes a number of available requests of 0 or less)

@rpkilby
Copy link
Member

rpkilby commented Aug 12, 2019

Many thanks for the response @diox.

@rpkilby rpkilby merged commit 5a8736a into encode:master Aug 12, 2019
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: '>' not supported between instances of 'float' and 'NoneType'
5 participants