1

I have one class that needs to be a singleton:

private static StationsFile instance;
private Context ctx;

protected StationsFile(Context ctx){
    this.ctx = ctx;

    load();
}
public static synchronized StationsFile getInstance(Context ctx){
    if(instance == null){
        Log.d("StationsFile", "set instance " + StationsFile.class.hashCode());
        instance = new StationsFile(ctx);
        Log.d("StationsFile", "instance set " + instance.hashCode());
    }
    return instance;
}
protected static StationsFile getInstance(){
    return getInstance(null);
}

private void load(){
    if(externalStorageIsReadable()){
        // Loads from sd file
    }else{
        loadDefaults();
    }
}

private void loadDefaults(){
    if(this.ctx != null){
        // Load from raw file in raws folder (I need a context to access Resources)
    }else{
        // Hardcoded default here
    }
}

Now, as this method is synchronized and it's static, it's supposed to be called only once at a time. When I run this in my device (Android 4.4) there's no problem (in my log i get "set instance - instance set"), but when I run it for the first time (and only the first time after installing) in a Android 2.2 Virtual Device or everytime in a Android 4.4 Virtual Device, I get in my log "set instance - set instance - instance set - instance set", along with a crash because of this.

So it looks like when the start is "slow" it crashes. To me it looks like the synchronized keyword just doesn't work.

This method gets called from 2 different threads at the very start of the app. One thread downloads and parses the file from the internet, so it gets updated, and the other thread (main thread) just reads it. (I don't need one to happen before the other, because if it's not loaded from internet it just reads it from the internal memory).

Is it a bug from the virtual devices? I mean, as it's synchronized (and as far as I understood) this can't be called twice at the same time. How can I fix this?

Edit: refractoring my class and making it follow the singleton pattern magically fixed it. Before I had public static methods that accessed getInstance so "I don't have to put getInstance everywhere I want to call this class". Eventhough, I'm going to rollback my code to find out what was the problem (I'm using a git repo).

Edit2: Using the hashcodes, i get the log: "set instance 1139286928 - set instance 1139286928 - instance set 1139224312 - instance set 1139287568"

Edit3: Found the bug. The problem was that the method loadDefaults when he loaded the file in the Resourcers folder, he parsed that file by using getInstance() again (calling a loadFromString method I had). I thought it were two different threads, but it was the same one, hence why synchronized didn't have any effect on it.

olivarra1
  • 3,269
  • 3
  • 23
  • 34
  • nobody can prevent the scheduler to context switching your first thread and lets run the second one, before getInstance() is completed executed. – Blackbelt Feb 24 '14 at 10:25
  • 4
    Are you sure you don't have two different classloaders involved for some reason? In that case, you'd have two independent Class objects. It would be worth logging something around that - e.g. `Log.d("StationsFile.class hash: " + StationsFile.class.hashCode()); – Jon Skeet Feb 24 '14 at 10:25
  • @JonSkeet Really intersting, but it's not the case. I tested that and I got the same hashcode in both calls (1139286840). – olivarra1 Feb 24 '14 at 11:12
  • 1
    Given that this isn't your actual code (you've said that your actual code has a parameter), can you post some code which exhibits this exact problem? It seems unlikely to me that synchronized really doesn't work. Also, you might try `synchronized (StationsFile.class) { ... }` within the method, which *should* be identical to the code you've got, but would at least be interesting to check. – Jon Skeet Feb 24 '14 at 11:20
  • Thank you so much. I found the bug, it was my fault. Explained in the 3rd edit. – olivarra1 Feb 24 '14 at 12:15

3 Answers3

1

Try this...

// initialized when the class is loaded for the first time
private static final StationsFile instance = new StationFile(); 

public static StationsFile getInstance() {
   return instance; // no need of null check here. No worry about synchronization
}

You might get what the difference is...

Gopal Gopi
  • 11,101
  • 1
  • 30
  • 43
  • you could also be so kind to explain what's the difference – Blackbelt Feb 24 '14 at 10:26
  • @blackbelt added some explanation. really thankful if you point out me about whether this answer is approximate or not... – Gopal Gopi Feb 24 '14 at 10:33
  • Very nice answer. It fixed the crash, but it's not 100% what I wanted. In my case I'm passing a parameter to getInstance (yea, very dirty, I know) for an optional argument in the constructor. And with this solution I'd to do getInstance().setParameter(param) which is not the ideal solution. – olivarra1 Feb 24 '14 at 11:16
0

So, as I said in the edits, the problem was because I was calling the synchronized method from the same method. It might sound like a recursion, but in this case it wasn't, since there was an optional parameter that made other effects.

If you get the same, just print/log your stack trace in the synchronized method to check wether it traces to another call to the same method.

And don't use static methods to avoid the getInstance() in the rest of the code like I did!

Community
  • 1
  • 1
olivarra1
  • 3,269
  • 3
  • 23
  • 34
0

Just make your instance variable final it will fix the problem:

private static final StationsFile instance;

"From Java 6 'Final' variables have special thread-safety semantics, in that other threads are guaranteed to see the final field in at least the state it was in when its constructor finished."

Mak
  • 596
  • 5
  • 10