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

After long inactivity period, available token counter becomes negative #51

Closed
MichelePaterniti opened this issue Sep 22, 2017 · 6 comments
Labels

Comments

@MichelePaterniti
Copy link

I noticed that, at least with some bandwidth configuration, when a long time passes between subsequent calls to Bucket.tryConsume(...), the available token counter becomes negative and Bucket.tryConsume(...) return false, while there should be lots of tokens available.

I guess this could be caused by an arithmetic overflow in executing line 136
long divided = refillTokens * durationSinceLastRefillNanos + roundingError;
in private method io.github.bucket4j.BucketState.refill(int, Bandwidth, long, long).

To reproduce the problem, you can use the following test:

import java.time.Duration;

import org.junit.Test;

import io.github.bucket4j.Bandwidth;
import io.github.bucket4j.Bucket;
import io.github.bucket4j.Bucket4j;
import io.github.bucket4j.TimeMeter;

import static org.junit.Assert.assertTrue;

public class LongWaitTest {
    
    private final class TimeMeterWithArtificialDelay
            implements TimeMeter {
        private static final long serialVersionUID = 286427698074660314L;
        
        final long twelveHourNanos = 12 * 60 * 60 * 1_000_000_000L;
        private long delay = 0L;
        
        @Override
        public long currentTimeNanos() {
            return TimeMeter.SYSTEM_MILLISECONDS.currentTimeNanos() + delay;
        }
        
        void let12HoursPass() {
            delay = twelveHourNanos;
        }
    }
    
    @Test
    public void whenALongTimePassesThenAvailableTokensCounterBecomesNegative() throws InterruptedException {
        final Bandwidth limit1 = Bandwidth.simple(700000, Duration.ofHours(1));
        final Bandwidth limit2 = Bandwidth.simple(14500, Duration.ofMinutes(1));
        final Bandwidth limit3 = Bandwidth.simple(300, Duration.ofSeconds(1));
        final TimeMeterWithArtificialDelay customTimeMeter = new TimeMeterWithArtificialDelay();
        final Bucket bucket = Bucket4j.builder()
                .addLimit(limit1)
                .addLimit(limit2)
                .addLimit(limit3)
                .withCustomTimePrecision(customTimeMeter)
                .build();
        
        assertTrue(bucket.getAvailableTokens() > 0);
        assertTrue(bucket.tryConsume(1));
        
        customTimeMeter.let12HoursPass();
        
        assertTrue("Free tokens expected after long wait", bucket.getAvailableTokens() > 0);
        assertTrue("Free tokens expected after long wait", bucket.tryConsume(1));
    }
    
}
@vladimir-bukhtoyarov
Copy link
Collaborator

long divided = refillTokens * durationSinceLastRefillNanos + roundingError;

Yes, you properly detected the source of problem. I will release the fix in few days.

@vladimir-bukhtoyarov
Copy link
Collaborator

Which version do you use?

@MichelePaterniti
Copy link
Author

I am using bucket4j-core 2.1.0 and bucket4j-jcache 2.1.0.

@vladimir-bukhtoyarov
Copy link
Collaborator

vladimir-bukhtoyarov commented Sep 22, 2017

Looks like with your limits bucket will work normally only for 3,6 hours between invokation

public static void main(String[] args) {
        double d = Long.MAX_VALUE;
        System.out.println(d  / (700000L * 1_000_000_000L * 3600L));
    }

. As dirty work-around you can touch the bucket periodically:

if (bucket.tryConsume(1)) {
    bucket.addTokens(1);
}

The fixes will be prepared for branches 2.0, 2.1, 3.0, most likely tommorrow.

@vladimir-bukhtoyarov vladimir-bukhtoyarov changed the title After long wait, available token counter becomes negative After long inactive period, available token counter becomes negative Sep 22, 2017
@vladimir-bukhtoyarov vladimir-bukhtoyarov changed the title After long inactive period, available token counter becomes negative After long inactivity period, available token counter becomes negative Sep 22, 2017
vladimir-bukhtoyarov added a commit that referenced this issue Sep 23, 2017
vladimir-bukhtoyarov added a commit that referenced this issue Sep 23, 2017
# Conflicts:
#	README.md
#	bucket4j-benchmarks/pom.xml
#	bucket4j-core/pom.xml
#	bucket4j-jcache/pom.xml
#	bucket4j-jcache/src/main/java/io/github/bucket4j/grid/jcache/JCacheCommand.java
#	bucket4j-parent/pom.xml
#	pom.xml
#	release-reactor/pom.xml
vladimir-bukhtoyarov added a commit that referenced this issue Sep 23, 2017
# Conflicts:
#	README.md
#	bucket4j-benchmarks/pom.xml
#	bucket4j-core/pom.xml
#	bucket4j-jcache/pom.xml
#	bucket4j-jcache/src/main/java/io/github/bucket4j/grid/jcache/JCacheCommand.java
#	bucket4j-parent/pom.xml
#	pom.xml
#	release-reactor/pom.xml
vladimir-bukhtoyarov added a commit that referenced this issue Sep 23, 2017
# Conflicts:
#	README.md
#	bucket4j-benchmarks/pom.xml
#	bucket4j-core/pom.xml
#	bucket4j-core/src/main/java/io/github/bucket4j/BucketExceptions.java
#	bucket4j-core/src/main/java/io/github/bucket4j/Refill.java
#	bucket4j-jcache/pom.xml
#	bucket4j-jcache/src/main/java/io/github/bucket4j/grid/jcache/ExecuteProcessor.java
#	bucket4j-parent/pom.xml
#	experimental/pom.xml
#	pom.xml
vladimir-bukhtoyarov added a commit that referenced this issue Sep 23, 2017
# Conflicts:
#	README.md
#	bucket4j-benchmarks/pom.xml
#	bucket4j-core/pom.xml
#	bucket4j-core/src/main/java/io/github/bucket4j/BucketExceptions.java
#	bucket4j-core/src/main/java/io/github/bucket4j/Refill.java
#	bucket4j-jcache/pom.xml
#	bucket4j-jcache/src/main/java/io/github/bucket4j/grid/jcache/ExecuteProcessor.java
#	bucket4j-parent/pom.xml
#	experimental/pom.xml
#	pom.xml
@vladimir-bukhtoyarov
Copy link
Collaborator

@MichelePaterniti please use 2.1.1(or 3.0.2) version.

@MichelePaterniti
Copy link
Author

Thanks for the promptness of your fix.
It took us some time to integrate it and to run all tests and checks, but now it is in production and it is working fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants