I've been struggling with java mutexing for a while now, I'm trying to have ByteIterator objects each print a new (and unique) byte[] they get from the JobQueue, but they continually print repeat values
How can i make sure only one thread enters the static method JobQueue.getByte() at a time?
Heres what i've tried:
ByteIterator and the JobQueue static methodand the code in question (getByte()):
public class JobQueue{
private static volatile Byte[] nextByte;
private static final Byte realMin = 0; // fixed values for debuging
private static final Byte realMax = (byte) 89;
public static void initJobQueue(){
nextByte = new Byte[ByteIterator.fleet.length];
for (int i=0; i<nextByte.length; i++) nextByte[i]=realMin;
}
//FIXME: this is the error prone method!
// This section should only be accessed by one thread at a time
// (ie none recive a duplicate byte)
public static Byte[] getByte(){
if(nextByte==null || checkEndVal(nextByte)) return null;
Byte[] currentByte = nextByte;
// iterate the value
for (int i = 0; i < nextByte.length; i++) {
if (nextByte[i]==realMax){
nextByte[i]=realMin;
} else{
nextByte[i]++;
break;
}
}
if(checkEndVal(nextByte)) nextByte=null;
return currentByte;
}
The running code and thread obj
import java.util.Arrays;
public class ByteIterator implements Runnable{
// ---- Game state settings (public and fixed for debugging)
public static final int[] fleet = {2,3};
public static final int BOARD_SIZE = 5;
public static final String lockingObject="test";
public static void main(String[] args) {
int threadCount = 2;
JobQueue.initJobQueue(); // setup the jobQueue
// ---- Create the threads
Thread[] threadArray = new Thread[threadCount];
for(int i=0; i<threadCount; i++){
threadArray[i] = new Thread(new ByteIterator());
threadArray[i].start();
}
}
@Override
public void run(){ // prints each next byte array
Byte[] foo = JobQueue.getByte();
while(foo != null){
System.out.println(Arrays.toString(foo));
synchronized (lockingObject) {
foo = JobQueue.getByte();
}
}
}
}
You're making all sorts of mistakes. No single fix makes it work because you have multiple errors; you need to fix all of them.
Don't do that. Locks are incredibly hard to reason about as is, if you make the lock object 'public', as in, tons of code all over the system might hold a ref to that object, you're making it from 'difficult' to 'utterly impossible'. Locking on a string is therefore a capital mistake, do not, ever, do that.
Make your own lock objects. And make sure they are private. And make sure that everything that needs to lock on it, locks on the same object. In some iterations of this question (you've been editing it), you had a non-static field, meaning each lockingObject field in every instance of ByteIterator was pointing at a different object. synchronized(x) does nothing except interact with other synchronized(z) statements, where z and x are pointing at the same object (z == x would be true).
Your first call in ByteIterator's run is getByte(), which isn't in any synchronized block. Only the while loop that follows does this.
Clearly the locking behaviour should occur in getByte(). Not in your ByteIterator run() method.
Byte is the wrapper type. You don't want that unless you really, really know what you are doing. You write byte[]. No, Byte and byte are not synonyms. Byte bad, byte good (later on in your java career you might learn when Byte is actually desired. It isn't, here).
Count the amount of new that will be executed in your code. That many objects exist. And not a single one more.
So let's go counting. You make a bunch of new ByteIterator objects, (1 per threadCount), as well as threads, but that's not what we are interested in. We're interested in Byte[] objects.
Your JobQueue class has a static initJobQueue method. This method is invoked only once. This method executes some form of new Byte[] only once.
End of count.
That means there is only 1 Byte array FOR YOUR ENTIRE APPLICATION.
All threads are sharing that byte array. Every write you make? It's all going to the same memory position. Every thread is jus writing all over each others business and the end result is a coinflippy mess (in the sense that every run will get you likely different results, the way threads interact is usually highly irregular, i.e. not repeatable).
Get every attempt to lock in any form out of your main class and ByteIterator.
Then:
public class JobQueue {
private static volatile byte[] nextByte;
private static final byte realMin = 0; // fixed values for debuging
private static final byte realMax = (byte) 89;
private static final Object lock = new Object(); // private!
public static void initJobQueue() {
nextByte = new Byte[ByteIterator.fleet.length];
for (int i=0; i<nextByte.length; i++) nextByte[i]=realMin;
}
public static Byte[] getByte() {
synchronized (lock) {
if(nextByte==null || checkEndVal(nextByte)) return null;
// Byte[] currentByte = nextByte; -- THIS DOES NOT MAKE A COPY --
byte[] currentByte = Arrays.copyOf(nextByte);
// iterate the value
for (int i = 0; i < nextByte.length; i++) {
if (currentByte[i]==realMax) {
currentByte[i]=realMin;
} else {
currentByte[i]++;
break;
}
}
if(checkEndVal(currentByte)) nextByte=null;
nextByte = currentByte;
return currentByte;
}
The crucial clue is Arrays.copyOf. As per the javadoc, that makes a new array. The first call to getByte() will dutifully run synchronized (one-at-a-time) and returns a byte array back to its caller. Your code will then, while the first thread is still working on its byte array, call getByte() again which would write all over that one single byte array, causing a mess. This code does not do that - instead it makes a clone (Arrays.copyOf runs new byte[] somewhere inside it, so, that's +1), and then modifies that clone, and then returns that clone.
Every byte array that is returned by getByte() is never modified by anything except, possibly, the code you gave it to.