Bug: Throttler does not show correct token time until 1 token is available

[rumpfc]

PHP Version

8.0

CodeIgniter4 Version

4.1.5

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Windows

Which server did you use?

cli-server (PHP built-in webserver)

Database

No response

What happened?

After some experiments with Throttler where I execute an action and get tokenTime returned after each action, I tackled some unexpected behaviour where I executed a successful action too early.

Example:

1. Action
2. token time: 50 seconds
3. wait 5 seconds
4. Action
5. token time: 45 seconds
6. wait 5 seconds
7. Action
8. Successful (even though there should be 40 seconds)

After some more experiments I realized that tokenTime is calculated using timestamp of last successful check instead of calculating when 1 token is available again.

Steps to Reproduce

Here is a unit test which uses a capacity of 2 and a refill timer of 200. This means

• 1 token every 100 seconds
• 0.01 tokens per second

It measures the token time after doing following steps:

1. check
2. sleep for 3 seconds
3. check
4. check (here it should expect false)
5. Expect token time 100 - 3 seconds = 97 seconds

<?php

use CodeIgniter\Test\CIUnitTestCase;
use Config\Services;

class ThrottlerTest extends CIUnitTestCase
{
    public function testThrottlerTokenTime()
    {
        $throttler = Services::throttler();

        $seconds = 200;
        $capacity = 2;
        
        // refresh = 200 / 2 = 100 seconds
        // refresh rate = 2 / 200 = 0.01 token per second

        // token should be 2
        $this->assertTrue($throttler->check('test', $capacity, $seconds));
        // token should be 2 - 1 = 1

        // do nothing for 3 seconds
        sleep(3);
        // token should be 1 + 3 * 0.01 = 1.03
        $this->assertTrue($throttler->check('test', $capacity, $seconds));
        // token should be 1.03 - 1 = 0.03
        $this->assertFalse($throttler->check('test', $capacity, $seconds));
        // token should still be 0.03 because check failed
        
        // expect remaining time: (1 - 0.03) * 100 = 97
        $this->assertEquals(97, $throttler->getTokenTime(), 'Wrong token time');
        
    }
}

?>

Here is the test output

PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

Warning:       XDEBUG_MODE=coverage or xdebug.mode=coverage has to be set

F                                                                   1 / 1 (100%)

Time: 00:03.125, Memory: 10.00 MB

There was 1 failure:

1) ThrottlerTest::testThrottlerTokenTime      
Wrong token time
Failed asserting that 100 matches expected 97.

C:\Users\chris\Documents\projects\php\spotify-web-party\tests\unit\ThrottlerTest.php:31

FAILURES!
Tests: 1, Assertions: 4, Failures: 1.

Expected Output

Throttler::tokenTime should return the remaining time until 1 token is available in case check failed.

Anything else?

Looking at your source code, I saw that you don't use consistent time. Sometimes you use time() (php function) and sometimes you use Throttler::time() (which uses either test time or time() from php). I tried above tests using $throttler-setTestTime(...), but got assertion errors not at tokenTime, but at the 2nd check.

[kenjis]

@rumpfc Thank you for reporting.

// token should still be 0.03 because check failed
// expect remaining time: (1 - 0.03) * 100 = 97

Why do you think so?

[kenjis]

Looking at your source code, I saw that you don't use consistent time. Sometimes you use time() (php function) and sometimes you use Throttler::time() (which uses either test time or time() from php).

Thanks! I sent a PR to fix it: #5463

[rumpfc]

@rumpfc Thank you for reporting.

// token should still be 0.03 because check failed
// expect remaining time: (1 - 0.03) * 100 = 97

Why do you think so?

I debugged through it 5 seconds after my last successful check. With a rate of 0.01/s and a token value of 0.87 I did the math to calculate when my token value is 1:

0.87 + 0.01*x = 1
0.01*x = 0.13
x = 13 seconds

I compared that result with $newTokenAvailable, but it showed me 95 seconds.

[kenjis]

Failed asserting that 100 matches expected 97.

Do you say it shows 100, but if you wait for only 97 seconds, you get a token in reality?

I think you should wait for 100 seconds or more.

[rumpfc]

Failed asserting that 100 matches expected 97.

Do you say it shows 100, but if you wait for only 97 seconds, you get a token in reality?

Exactly, the token is already available after 97 seconds, 3 seconds earlier then actually displayed. (Tried out with version 4.1.5)

There is a game using a similar mechanism: Dota 2. Some skills have charges. The moment you don't have max. number of charges, a timer starts, showing when you get the next charge for your skill.

I assume that CI throttler should work the same

[kenjis]

@rumpfc I sent a PR: #5470

By the way, don't you want to give penalty to users who accessed when running out of token?
For example, I think it is okay that the last test in your test code returns 100 (really need to wait 100 sec).

[rumpfc]

By the way, don't you want to give penalty to users who accessed when running out of token?
For example, I think it is okay that the last test in your test code returns 100 (really need to wait 100 sec).

I see it more like pocket money for your children so that they can use it however they want. Every week/month they get a fixed amount, no matter if they still have it or already used it up.

If the children ask you for more money now even though they would get their pocket money in 2 days anyway, would you say "No, and now you have to wait 30 days to get your next pocket money"?

I'd tell them to wait patiently because they would get their fair amount regularly anyway.

If you want to punish your users in that way, just change the parameters for capacity and seconds or increase the costs for that user 😉

[kenjis]

@rumpfc Okay, you don't have needs to increase refresh time.
I understand the current behavior (expect wrong token time) is okay to you.

[rumpfc]

@rumpfc Okay, you don't have needs to increase refresh time.
I understand the current behavior (expect wrong token time) is okay to you.

Exactly. For me:

• Correct token behavior
• Wrong tokenTime calculation

The current logic is a mix of both behaviors (your view and my view). This cannot work by a long shot. Either fix tokenTime or fix token. You know my opinion, but I give the main contributors the final call.

[kenjis]

@rumpfc
At leaset the current tokenTime is incorrect in your test case. I will fix it.

But I'm not sure what is tokenTime. What do you think of this?
#5470 (comment)

[rumpfc]

@rumpfc
At leaset the current tokenTime is incorrect in your test case. I will fix it.

But I'm not sure what is tokenTime. What do you think of this?
#5470 (comment)

I want to quote from official CI4 manual about token time:

After check() has been run and returned false, this method can be used to determine the time until a new token should be available and the action can be tried again. In this case, the minimum enforced wait time is one second.

It seems you want that the token mechanism works like this: After the user used up all his tokens, he/she must wait xxx seconds to get a new set of tokens again.

Imagine you have 3 tokens available which refreshes every 1 hour. You use 2 tokens (now having 1 left) and you do nothing for 55 minutes. Now he/she uses the final token (having no tokens left and 5 minutes before token refreshes). Do you still think the user should get no tokens for another 1 hour just because he/she wants to do something too early?

[kenjis]

@rumpfc Thank you for quoting.
According the CI manual, tokenTime can not be used (must be 0?) when check() returns true.

[rumpfc]

According the CI manual, tokenTime can not be used (must be 0?) when check() returns true.

How about returning 0, -1 or null (making the return type nullable)?

[kenjis]

Changing the return type is a breaking change.
And I don't understand what -1 means.

[rumpfc]

Changing the return type is a breaking change.

Honestly I don't see the change from int to ?int as a breaking change. More like taking advantage of a PHP feature. 😉

And I don't understand what -1 means.

0 or a negative number can mean

• not set
• no restrictions
• not used
• error
• etc.

Depends on your definition. Just explain in the documentation the meaning of your return value.

[kenjis]

@rumpfc
You are correct. Changing from int to ?int is not a breaking change.
https://3v4l.org/Ss8kl

I think 0 is enough for now. 0 means you should not have got it, or it is not calculated.

[rumpfc]

@kenjis don't forget to update the document. I think it still mentions "1" on check() = true

[kenjis]

@rumpfc Where in the document?

[rumpfc]

In the User Guide and in PHPDoc at the method

https://github.com/codeigniter4/userguide/blob/master/docs/libraries/throttler.html#L519

[kenjis]

@rumpfc

After check() has been run and returned false, this method can be used to determine the time until a new token should be available and the action can be tried again. In this case, the minimum enforced wait time is one second.
https://codeigniter4.github.io/CodeIgniter4/libraries/throttler.html#getTokentime

I don't get what you mean.
I think the explanation is correct. I interpret that when check() returned true, you can not use it.
The document says nothing about the return value of the getTokentime() when check() returned true.

[rumpfc]

@kenjis I see.