5

Ok, so I have a monitoring thread that checks a ArrayList size and does something after that size goes greater than a certain number. The problem I am having right now is the size value is never updated unless I have a print statement in my loop. Here is some code to show what exactly I have going.

while(working) {
    // Get size function just returns the size of my list in my t class
    int size = t.getSize();
    if (size >= 10) {
        //DO STUFF
    }
}

This above code does not work. It never goes into the if statement. However, this works fine:

while(working) {
    // Get size function just returns the size of my list in my t class
    int size = t.getSize();
    System.out.println(size);
    if (size >= 10) {
        //DO STUFF
    }
}

EDIT: getSize() code:

public ArrayList<byte[]> myQueue = new ArrayList<byte[]>();

public int getSize() {
    return myQueue.size();      
}

NOTE: I have another thread running that is updating and adding to my list in my t class.

Any help? this is really annoying to have it spitting out numbers when I am trying to debug in the console.

ageoff
  • 2,798
  • 2
  • 24
  • 39
  • put a System.out inside if (size >= 10) { //DO STUFF } and check – Shreyos Adikari May 30 '13 at 14:30
  • 2
    Sounds like a race hazard to me - are you correctly synchronizing on calls to arraylist? – Michael Berry May 30 '13 at 14:30
  • The only thing that changes between your working and non working program is the println? – Djon May 30 '13 at 14:31
  • try to `synchronize(t)` inside the loop. I think this will get the correct value. – joaonlima May 30 '13 at 14:32
  • 1
    Show us the code of `getSize()`. – Eng.Fouad May 30 '13 at 14:33
  • 2
    In addition to proper synchronization, make sure your shared state is `volatile`. – nicholas.hauschild May 30 '13 at 14:34
  • 2
    @nicholas.hauschild and joaonlima, is volatile needed? The doc about synchronized states that "This guarantees that changes to the state of the object are visible to all threads.". http://docs.oracle.com/javase/tutorial/essential/concurrency/syncmeth.html – Djon May 30 '13 at 14:45
  • @Djon Each thread can keep its state in a local cache. Using `volatile` tells the JVM to NOT use this cache and to instead always get the latest value. It is not necessary to use if it is not necessary to always have the most up to date value (in a multi threaded environment). – nicholas.hauschild May 30 '13 at 14:49
  • I understand, thank you. Does it mean that I probably don't need to make an object volatile if its reference is unlikely to change? – Djon May 30 '13 at 14:52
  • 1
    @Djon You are right. I've just revisited Josh Bloch's "Effective Java" and he states that only synchronized is needed. Or only volatile, if there is no need for mutual exclusion. – joaonlima May 30 '13 at 14:52
  • @nicholas.hauschild If you make an answer that says make my list volatile i will accept it. That worked fine. – ageoff May 30 '13 at 15:04

4 Answers4

6

If the only thing changing between your working and non working code is the println statement, then you almost certainly have a threading issue. The System.out.println() statement adds a small pause which may coincidentally cause it to behave, but is not solving the issue. You could do something like:

try {
    Thread.sleep(10);
} catch (InterruptedException ex) {
}

...in place of this and check for the same behaviour, if indeed it is the same then this pretty much confirms the threading issue. However, as pointed out below println() also does a few other things such as causing a memory barrier, so this isn't a foolproof test. Another, perhaps better check could be to temporarily swap out ArrayList for Vector, which is thread safe - though this is a legacy collection so not recommended for use in the final code.

If this is the case, it sounds like you're not synchronising on ArrayList calls properly - ArrayList is not a thread safe collection. Whenever you read or write to the list, do it inside a synchronized block like so, synchronizing on the list:

synchronized(list) {
    list.whatever();
}

...which will ensure that only one thread can access the ArrayList at once, hopefully solving the threading issue.

Michael Berry
  • 70,193
  • 21
  • 157
  • 216
  • 2
    The sysout call does even more: it is synchronized and causes a memory barrier to occur, which is effectively the same as reading a volatile var. – Marko Topolnik May 30 '13 at 14:42
  • Yes, I dont know where the answer went (he must haev deleted it) but simply adding the volatile tag to my list did the trick. Is this a "safe" solution? I feel like it is too simple. – ageoff May 30 '13 at 14:58
  • @LiverpoolFTW I wouldn't say it's the best solution - volatile has more to do with visibility than execution sequence. It *may* well make it thread safe (I don't really know enough about the specifics to say), but synchronizing is much clearer as to what's going on, and *definitely* safe. – Michael Berry May 30 '13 at 15:02
5

ArrayList is not synchronized, or otherwise prepared for use in two threads at once. In JLS terms, there is no happens-before relationship between the addition of elements in one thread and a size() call in another thread.

The cleanest solution would be to use a synchronized List implementation. You can either use Vector, or create a synchronized wrapper around your ArrayList using Collections.synchronizedList().

Patricia Shanahan
  • 25,849
  • 4
  • 38
  • 75
  • +1, but I wouldn't use `Vector` - it synchronises on each individual operation which is almost never what's needed. `Collections.synchronizedList()` or manual synchronisation also states the intent much better than using a legacy collection, I've seen people swap out Vector for ArrayList before just because they think the latter is the drop in newer version of the former. – Michael Berry May 30 '13 at 14:47
2

Your reader is starving the writer so it never gets a chance to run, never adding anything to your list.

By adding a I/O call (system.out.print or Thread.sleep) you put the reader thread in a blocking state which allows other one to run.

Generally, loops which consumes 100% CPU like this is bad. At least add a short sleep/yield somewhere in the loop.

Martin Wickman
  • 19,662
  • 12
  • 82
  • 106
1

Without seeing any other code there are a number of things that can be going on here. If you give us a short reproduce able example that may help.

As far as 'a number of things' It is possible that the the compiler can re-order the write of size outside of the while loop for instance

int size = t.getSize();
while(working){
   if(size >= 10){ 

   }
}

But again that is just speculation at this point.

John Vint
  • 39,695
  • 7
  • 78
  • 108
  • This will never change the value of size – joaonlima May 30 '13 at 14:42
  • @joaonlima It won't, but if every time the `getSize()` is initially invoked it is < 10 then the if will never be satisfied – John Vint May 30 '13 at 14:43
  • Sure. But imo the point is the constantly refresh the `value` and perform some active if needed – joaonlima May 30 '13 at 14:48
  • 1
    I understand. My point is that because of reording and lack of synchronization there is no guarantee the size value will ever be updated. Hoisting is a similar problem – John Vint May 30 '13 at 14:51