Thread safety & synchronized

Newbie & Debugging Questions
#1

Hello, I have a problem with thread safety on this block of code. Synchronized just seems to be ignored… I am using a static variable to count the calls to it, and it’s reaching 2 most of the times, while it should never go over 1. Am I doing something incredibly and terribly wrong or is this a bug? I also tried making the whole method synchronized, and it won’t change anything.

    //This method looks for an available model id. If none is found, it tries to discard an expendable model to make room.
    //If no expendable model, it returns -1 as an error code, and the model won't be loaded.
    public int createInFirstAvailableID(String fileName){
        int i=0;
      synchronized (this){
            
        ModelsManager.count++;
        System.out.println("Istances: "+ModelsManager.count);
        while(i<Configuration.getMaxModelCount() && models[i++]!=null);
        
        if(i<Configuration.getMaxModelCount()){ //I found an usable id
            System.out.println("Id found: "+ (i-1)+" for "+fileName);
            models[--i]=new Model3D(fileName);
            System.out.println("Model created: "+ i+" for "+fileName);

        ModelsManager.count--;   
            return i;
        }else{ //No available id, need to discard an expendable texture
            //if I get here it means all ids are taken, pointless to check on models[] again, just free up one with 0 istances
            i=0;
            while(i<Configuration.getMaxModelCount()){
                if(instanceCount[i]==0){  //Found one to discard
                    models[i].release();
                    models[i]=new Model3D(fileName);                    
        ModelsManager.count--;            
                    return i;                    
                }else{
                    i++;
                }                
            }
        ModelsManager.count--;            
            return -1;
        }    
        }
    }

Thanks…

#2

Ok… since it didn’t make any sense to have a threading problem there… I went to find out if the lock I was acquiring was really the same… and found this idiotic mistake…
The code was in a singleton class, and I made my mistake on the getInstance() method, where I forgot to specify the MyClass identifier while using the singletonInstance reference. This didn’t fail to compile (as singletonInstance IS a member of the class, even if static) but didnt actually work as a static variable should. I don’t really understand where the variable goes, if it’s not in the “this” reference, it’s not in the method stack and it’s not a real static variable, then what is it? But ok, adding that fixed it.

Wrong code:

  
public class MyClass{
  //Singleton instance pointer
    private static MyClass singletonInstance=null;
    
    protected MyClass() {
    }
    
    public static MyClass getInstance(){
        if(singletonInstance==null){
            singletonInstance=new MyClass();
        }        
        return singletonInstance;        
    }

Correct code:

  

public class MyClass{
  //Singleton instance pointer
    private static MyClass singletonInstance=null;
    
    protected MyClass() {
    }
    
    public static MyClass getInstance(){
        if(MyClass.singletonInstance==null){
            MyClass.singletonInstance=new MyClass();
        }        
        return MyClass.singletonInstance;        
    }

Please let me know if what I typed here has some minor/major imprecisions… or if anyone know why the first case still compiles.
Thanks.

#3

Fine fine… so after all it doesn’t make any difference to use the first or the second code… The problem instead lies in the fact the getInstance method needs to be synchronized, what was happening was two different threads were calling it at the same time, and they actually instanced two different instances of this singleton class. By maintaining their own references to the returned instance, they would use two different instances, thus acquiring two different locks, and making a big mess later, when a new call to the getInstance method would cause the loss of this unwanted second instance.

Finally the fixed code…

public class MyClass{
  //Singleton instance pointer
    private static MyClass singletonInstance=null;
   
    protected MyClass() {
    }
   
    public synchronized static MyClass getInstance(){
        if(singletonInstance==null){
            singletonInstance=new MyClass();
        }       
        return singletonInstance;       
    }

Thanks to javaWorld on the flawed tutorial about singleton~
Thanks…

#4

do you often talk to yourself like this? ;D :stuck_out_tongue:

#5

i don’t really understand what the hell are you trying to do mauryr, but if you wonder about locks just create a Object only for locking on him…

Object LockName = new Object();
synchronized (LockName) {…}

#6

Yes, I do that very often… for some reason I spend hours on idiotic bugs, then I post a question and find the answer by myself…
To Kova: I already have my object I wanna lock on, the only problem was I thought there always was only a single instance of it, but there could be 2, which made 2 locks available, while I needed it to be singleton~ So the problem was not in the lock itself, but in the singleton pattern implementation, resulting flawed in this case because two different thread were concurrently calling the “side constructor” at the same time, thus creating two instances, one of which got lost after a few, losing important data which should have belonged to the one and only instance…