Return to Answer

edited for clarity

Source Link

edited Jun 23 at 15:37

BoundedConcurrentQueue.top() blocks indefinitely (infinite if the producers have terminated) if the queue is empty due to aquiring semaphoreFreeSpots. If the intention is indeed to just peek, it should 1) be called peek, 2) not aquire semaphoreFreeSpots and 3) return nullnull if the size field indicates that the queue is empty:

Another alternative would be to establish a new method top(long timeoutInMilis) that returns null or throws if the timeout expires without an element showing up.

The termination of the Consumers doesn't seem well-thought out to me. Clearly, currently the intention seems to be that they bail when they pop a halting element from the queue, however, nothing currently ensures that the correct number of halting elements is produced (Producers seem to be arbitrarely producing 0 or 1 depending on which if inside their run() method is hit, and in any case there could be an unequal number of producers and consumers). Indeed, with the following settings:

a number of consumers block infinitely at the end waiting inside top() for an element that will never arrive. If you implemented the top()top() suggestion above, that method returning nullnull would be a good opportunity to check this.sharedState.isHaltRequested() (that field is inherited from the superclass and can IMO stay there, although they do would need to initialise it properly in that case) on wether its time to bail out. Otherwise, I think the best way would be to have the producers become aware of how many consumers there are via a AtomicInteger field in sharedState and have them work together to produce exactly the correct number of halting elements.

Since all those setters in Simulator set stuff thats ultimatively required, move them to the constructor (or, better, use a builder pattern).

BoundedConcurrentQueue.top() blocks indefinitely (infinite if the producers have terminated) if the queue is empty due to aquiring semaphoreFreeSpots. If the intention is indeed to just peek, it should 1) be called peek, 2) not aquire semaphoreFreeSpots and return null if the size field indicates that the queue is empty:

Another alternative would be to establish a new method top(long timeoutInMilis) that returns or throws if the timeout expires without an element showing up.

The termination of the Consumers doesn't seem well-thought out to me. Clearly, currently the intention seems to be that they bail when they pop a halting element from the queue, however, nothing currently ensures that the correct number of halting elements is produced (Producers seem to be producing 0 or 1 depending on which if inside their run() method is hit, and in any case there could be an unequal number of producers and consumers). Indeed, with the following settings:

a number of consumers block infinitely at the end waiting inside top() for an element that will never arrive. If you implemented the top() suggestion above, that method returning null would be a good opportunity to check this.sharedState.isHaltRequested() (that field is inherited from the superclass and can IMO stay there, although they do would need to initialise it properly in that case) on wether its time to bail out. Otherwise, I think the best way would be to have the producers become aware of how many consumers there are via a AtomicInteger field in sharedState and have them work together to produce exactly the correct number of halting elements.

Since all those setters in Simulator set stuff thats ultimatively required, move them to the constructor (or, better, use a builder).

BoundedConcurrentQueue.top() blocks indefinitely (infinite if the producers have terminated) if the queue is empty due to aquiring semaphoreFreeSpots. If the intention is indeed to just peek, it should 1) be called peek, 2) not aquire semaphoreFreeSpots and 3) return null if the size field indicates that the queue is empty:

Another alternative would be to establish a new method top(long timeoutInMilis) that returns null or throws if the timeout expires without an element showing up.

a number of consumers block infinitely at the end waiting inside top() for an element that will never arrive. If you implemented the top() suggestion above, that method returning null would be a good opportunity to check this.sharedState.isHaltRequested() (that field is inherited from the superclass and can IMO stay there, although they do would need to initialise it properly in that case) on wether its time to bail out. Otherwise, I think the best way would be to have the producers become aware of how many consumers there are via a AtomicInteger field in sharedState and have them work together to produce exactly the correct number of halting elements.

Since all those setters in Simulator set stuff thats ultimatively required, move them to the constructor (or, better, use a builder pattern).

Source Link

answered Jun 23 at 8:12

Jannik S.

LongElementProvider.provide() makes me nervous:

public Long provide() {
    if (numberOfOperationsPerformed++ < totalNumberOfOperations) {
        return ThreadLocalRandom.current().nextLong(bound);
    } else {
        return haltingElement;
    }
}

numberOfOperationsPerformed should probbably be an AtomicLong, since its accessed concurrently from multiple threads with no sign of a lock anywhere on the callstack.

I personally would favor it if the Threads had unique names, making it easier to discover what a particular thread is (Not so important in a simple project as this, but in larger projects it can be annoying if something doesn't terminate cleanly and all one's got is the default Thread-*n*). Perhaps add the following to the AbstractSimulationThread constructor?

    this.setName(this.getClass().getSimpleName() + "-" + this.threadId);

BoundedConcurrentQueue.top() blocks indefinitely (infinite if the producers have terminated) if the queue is empty due to aquiring semaphoreFreeSpots. If the intention is indeed to just peek, it should 1) be called peek, 2) not aquire semaphoreFreeSpots and return null if the size field indicates that the queue is empty:

public E peek() {
    mutex.acquireUninterruptibly();
    if (this.size == 0) {
        return null;
    }
    final E topElement = array[headIndex];
    mutex.release();
    return topElement;
}

Another alternative would be to establish a new method top(long timeoutInMilis) that returns or throws if the timeout expires without an element showing up.

The termination of the Consumers doesn't seem well-thought out to me. Clearly, currently the intention seems to be that they bail when they pop a halting element from the queue, however, nothing currently ensures that the correct number of halting elements is produced (Producers seem to be producing 0 or 1 depending on which if inside their run() method is hit, and in any case there could be an unequal number of producers and consumers). Indeed, with the following settings:

private static final int DEFAULT_NUMBER_OF_PRODUCERS = 6;
private static final int DEFAULT_NUMBER_OF_CONSUMERS = 12;
private static final int DEFAULT_QUEUE_CAPACITY = 12;
private static final long DEFAULT_HALTING_ELEMENT = -1;
private static final long LONG_BOUND = 62;
private static final long TOTAL_OPERATIONS = 40_000;

Since all those setters in Simulator set stuff thats ultimatively required, move them to the constructor (or, better, use a builder).