File APIs for Java Developers
Manipulate DOC, XLS, PPT, PDF and many others from your application.
http://aspose.com/file-tools
The moose likes Threads and Synchronization and the fly likes Is accessing static class variables from non-static methods bad practice? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Java » Threads and Synchronization
Bookmark "Is accessing static class variables from non-static methods bad practice?" Watch "Is accessing static class variables from non-static methods bad practice?" New topic
Author

Is accessing static class variables from non-static methods bad practice?

Sean Keane
Ranch Hand

Joined: Nov 03, 2010
Posts: 581

I've a question about the Data class below around synchronization. The class in question is a Singleton and controls access to a list of records.

Internally the class has a cache containing the list of records. This cache is loaded when a call is made to Data.getInstance(someFileLocation). The structure is something like this:


Now I have a few questions about this:

  • 1) I understand that because I am not using lazy initialisation for the singleton instance, then this class is thread safe. Is my understanding correct?
  • 2) I am populating my cache through a method that is synchronized on the class, so I am guaranteed that only one thread will be able to populate the cache at a time. Is my understanding here correct?
  • 3) I read somewhere that you should always use static synchronized methods to access static class variables, and use non-static (instance) synchronized methods to access instances variables. Is this true? If so, this means I have a problem right? Because I am populating my static class variable using a static synchronized method but I am updating records in the cache using synchronized instance methods.


  • Point (3) is really my main concern. I'm pretty sure there is a problem here. Because the following could happen

    A) Thread-A calls Data.getInstance("c:\datafile.dat"); .... the cache is loaded
    B) Thread-A calls data.update(1, new String[]{}); ....Thread-A gets suspended before the update method can actually update the record in the cache
    C) Thread-B class Data.getInstance("c:\datafile_other.dat");...the cache gets loaded with new contents, this is allowed because getInstance is a static method so it only requires the class lock, and Thread-A doesn't hold the class lock
    D) Thread-A is back running again and attempts to update the cache....but record 1 no longer exists as Thread-B replaced the contents of the cache.

    So how can I get around this problem. I'm a bit confused because

  • I want a singleton as I only want one instance of this class in my application.
  • My getInstance method has to be static, otherwise you'd have no way of getting an instance!
  • My methods that operate on the cache need to be non-static, otherwise they could be called before the cache is loaded.


  • If I make my cache a static class variable, then my methods such as read\update etc. that operate on the cache will be non-static methods accessing a static class variable.
    If I make my cache a non-static class variable, then my getInstance() methods will be static methods accessing non-static instance variables.

    So it's seems no matter what way I approach this, I will break the rule of "only have static methods access static fields, and instance methods access instance fields". What to do?


    SCJP (1.4 | 5.0), OCJP (6.0), OCMJD
    Pat Farrell
    Rancher

    Joined: Aug 11, 2007
    Posts: 4646
        
        5

    I've never heard of any rule of "only have static methods access static fields, and instance methods access instance fields". However, perhaps its really saying that you should use static getters and setters to access static fields.

    However, I question your basic assumptions. Singletons are a bad practice. Don't use that pattern. Its evil. There are many posts here in the Ranch and in serious programming forums and blogs all over the Internet saying that Singletons are evil.

    Implementing Singletons is not only bad practice, but its very hard to do properly. Google for the IBM papers on double-lock errors when creating singletons.

    Its trivial to use a Factory pattern and only create one instance, which accomplishes the good that a Singleton pattern claims to offer, without all of the very evil, bad, and smelly things that the Singleton pattern delivers in practice.
    Sean Keane
    Ranch Hand

    Joined: Nov 03, 2010
    Posts: 581

    Thanks for the reply Pat. Maybe a recommendation would be a better description rather than a rule. But I think the basic thought behind the idea was that if you use static methods to access static fields and instance methods to access instance fields then you've a greater chance your class will be thread safe. You'll avoid some problems, like the one I encountered in my previous post when both the static and non-static methods are able to access the same resource at the same time leaving me with a solution that is not thread safe.

    Double-Locking...

    I've read the IBM paper about double-locking before. I'm open to correction here, but I don't think it applies to my example - because I'm not using double locking. I'm assigning the Singleton instance directly to the variable which happens when the class is loaded - which I think is guaranteed to be thread safe.

    From what I've read on double-locking, this approach is utilised when putting the lock on the entire method is a concern due to performance. But this is not a concern of mine. So I think I can leave double-locking concerns to one side.

    Thread-Safe Singleton...

    Leaving aside the argument of whether a Singleton is a good idea or not. Say I do want to fire ahead with that idea - is it really that hard to create a thread-safe one properly?

    For a basic Singleton is it quite simple to make it thread safe as outlined here. Here's the extract below of a Singleton that is thread safe and I think you'd agree there is nothing hard about doing that?


    Making My Design Thread-Safe...

    So moving on, for my particular design I have a way of making it thread-safe - but I don't particularly like it .So I was looking for suggestions of ways that I could make it thread-safe.

    My solution for making my design thread-safe is to only have my getInstance() method return an instance that is not initialised - i.e. the cache is not loaded. Then the the loadRecordsToCache() instance method can be called on this instance. So I would have

    This solution is now thread safe because the only way to modify the cache is through the instance methods - all instance methods in my class are synchronized. The static method only returns an reference to the static class field i.e. the Singleton instance of the Data class.

    I don't like this solution because it means the CRUD methods will blow up if called before the cache is loaded. So I would rather my Singleton instance was returned with the cache already loaded. So the cache being loaded is like a class invariant - no cache loaded, then no instance available!


    What I Would Like...

    But what I would really like is to be able to do this in my code:


    ...in a thread-safe manner of course ! Any suggestions?
    Chris Hurst
    Ranch Hand

    Joined: Oct 26, 2003
    Posts: 416
        
        2

    Your singleton is fine, there are two obscure gotchas to be aware of that I'll just mention though I'm sure they won't effect you...

    i) Classloaders - you can have a singleton per class loader ...

    http://java.sun.com/developer/technicalArticles/Programming/singletons/

    ii) two stage construction is usually preferred for several reasons but purely from a threading point of view ...

    Adding complexity to a constructor runs the risk that some one either when coding or through later modification allows the 'this' pointer to escape (give visibility to the this pointer to another thread before the ctr completes) , you commonly see this is swing apps where they register a listener (EVEN as the last line in the constructor this allows bad and obscure things to happen e.g. visibility of finals not guaranteed) ....

    http://www.cs.umd.edu/~pugh/java/memoryModel/


    "Eagles may soar but weasels don't get sucked into jet engines" SCJP 1.6, SCWCD 1.4, SCJD 1.5,SCBCD 5
    Sergey Babkin
    author
    Ranch Hand

    Joined: Apr 05, 2010
    Posts: 50
    A thing to keep in mind is that synchronized and static synchronized will use different lock: the static methods will use a lock on the class, and the normal methods will use a lock on the instance. So they won't synchronize with each other. If you access the static variables through static synchronized methods, this will make sure that the access to them is synchronized.

    An easy way to synchronize in the getInstance() is to do the full initialization of the object before assigning it into the static reference. This way no other thread can see the object until it's initialized.

    And finally: don't use singletons. They are a very bad programming practice. Even worse than the simple global variables.
     
    I agree. Here's the link: http://aspose.com/file-tools
     
    subject: Is accessing static class variables from non-static methods bad practice?