Limit method call / seconds in multi-threaded environnement

Question

I'm developping mechanics to handle weapons for a game. I'm having trouble with the rate of fire, it works well with a single thread but it seems that my GUI is calling the fire() method from multiple thread which causes some unexpected behaviour sometimes (multiple shots are fired very quickly). So I added a lock in the method (with an AtomicBoolean flag) but it only causes the bug to appear less frequently.

Weapon class

private final AtomicBoolean locked = new AtomicBoolean(false);
private final AtomicInteger fired = new AtomicInteger(0);

@Override
// returns true if a shot was fired; false otherwise
public boolean fire() {
    if (locked.compareAndSet(false, true)) {
        return false;
    }
    if (!canFire || reloading)
        return false;

    if (bulletsInMagazine > 0) {
        canFire = false;
        timeSinceLastShot = 0;
        bulletsInMagazine--;
        fired.incrementAndGet();
        if (bulletsInMagazine == 0) {
            reload();
        }
        locked.set(false);
        return true;
    }
    reload();
    locked.set(false);
    return false;
}

@Override
public void tick(int ms) {
    timeSinceLastShot += ms;
    if (timeSinceLastShot >= delayBetweenShotsMs) {
        canFire = true;
    }

    remainingReloadTime = Math.max(0, remainingReloadTime - ms);
    if (remainingReloadTime == 0 && reloading)
        processReloading();
}

I wrote a unit test which reproduces the problem... sometimes.

Test:

@Test
public void testFiringRate() {
    int nbBullets = weapon.getBulletsInMagazine();
    ScheduledExecutorService scheduledExecutorService = Executors.newScheduledThreadPool(16);
    for (int i = 0; i < 16; i++) {
        scheduledExecutorService.scheduleAtFixedRate(weapon::fire, 5, 10, TimeUnit.MILLISECONDS);
    }
    System.out.println("START firing");
    int ticks = 0;
    while (ticks < 100) {
        weapon.tick(100);
        ticks++;
        try {
            Thread.sleep(10);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        int fired = nbBullets - weapon.getBulletsInMagazine();
        int reallyFired = weapon.getFiredShotCount();
        int diff = reallyFired - fired;
        if (diff != 0) {
            System.out.println(ticks + "\t" + fired + " shot(s)\t" + reallyFired + " really fired\t" + (diff != 0 ? "(+" + diff + ")" : ""));
        } else {
            System.out.print(".");
        }
    }
    System.out.println("\nSTOP firing");
    System.out.println(ticks + " Ticks");
    try {
        System.out.print("WAITING FOR ALL TASKS TO TERMINATE...");
        scheduledExecutorService.shutdown();
        scheduledExecutorService.awaitTermination(5, TimeUnit.SECONDS);
        System.out.println("DONE.");
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
    int bulletsAfter = weapon.getBulletsInMagazine();
    int fired = nbBullets - bulletsAfter;
    System.out.println("Shot fired: " + fired);
    int shotsFired = weapon.getFiredShotCount();
    System.out.println("Really fired: " + shotsFired);
    assertEquals(shotsFired, fired);
}

What I got (sometimes): The calculated shots is 1 or 2 less than reality (from the AtomicInteger fired)

What I want: no matter the number of threads, the weapon should respect its rate of fire.

Any idea ?

EDIT:

I tried to invert the if condition:

if (!locked.compareAndSet(false, true)) {
    return false;
}

It resolved the problem from unit test but now GUI fire only once and never again.

What about using a Semaphore? https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Semaphore.html — Sleiman Jneidi, Jan 04 '18 at 22:41
I was thinking that `if (locked.compareAndSet(false, true)) { return false; }` was acting like a semaphore... am I wrong ? — tinysquare, Jan 04 '18 at 22:45
Why don't you have an instance variable `rate` / `fired` and synchronize on it? you can do that inside your method `fire` using a block like `synchronized(this.rate){ //do what you want }` — ichantz, Jan 04 '18 at 23:08
You should look into using a game loop so you can avoid most threading issues. — David Ehrmann, Jan 05 '18 at 18:34
@DavidEhrmann Actually I have a game loop that runs 60 times/second. When the player clicks it just set a flag that tells game loop to start firing (and stop when flag is removed). I supposed multiple threads invocation because I had no other explanation (and thought I forgot to remove some temporary code). In fact the firing method was just buggy even on single thread I guess. — tinysquare, Jan 05 '18 at 22:08

score 0 · Accepted Answer · answered Jan 05 '18 at 18:15

0

I moved the method body in a synchronized(this) block as suggested by @ichantz and it works because I'm unable to reproduce the bug after many tries.

answered Jan 05 '18 at 18:15

tinysquare

1
3

Limit method call / seconds in multi-threaded environnement

1 Answers1