2

In the constructor of my class, I map the current object (this), along with its key (a string entered as a parameter in the constructor) into a static LinkedHashMap so I can reference the object by the string anywhere I might need it later.

Here's the code (if it helps):

public class DataEntry {
    /** Internal global list of DataEntry objects. */
    private static LinkedHashMap _INTERNAL_LIST;

    /** The data entry's name. */
    private String NAME;

    /** The value this data entry represents. */
    private Object VALUE;


    /** Defines a DataEntry object with a name and a value. */
    public DataEntry( String name, Object value )
    {
        if( _INTERNAL_LIST == null )
        {
            _INTERNAL_LIST = new LinkedHashMap();
        }

        _INTERNAL_LIST.put( name, this );

        NAME = name;
        VALUE = value;
    }
}

The problem? Instances of this class won't get garbage collected when I'm done using them.

I'm just curious if there's a way to have instances of this class clean themselves up when I'm done using them without having to manually call a Remove() method or something each time (to remove its reference in the internal LinkedHashMap when I'm no longer using them, I mean).

Daddy Warbox
  • 4,500
  • 9
  • 41
  • 56
  • Should the class work in environments that use threads? It won't at present. – McDowell Dec 06 '08 at 21:52
  • What's stopping it specifically? – Daddy Warbox Dec 06 '08 at 21:59
  • The static variable _INTERNAL_LIST is not synchronized. There is a chance that multiple threads will cause multiple maps to be initialized. Also, LinkedHashMap is not synchronized, so synchronous calls to put may leave the map in an unstable state. – McDowell Dec 06 '08 at 22:04
  • Actually, I can't envisage any situation where caching via the constructor is a good idea. – McDowell Dec 06 '08 at 22:10
  • 2
    Statics are bad. Exposing this in a constructor is bad. Not using final on these fields is bad. Using odd naming conventions is bad. Doing all the above is really bad. – Tom Hawtin - tackline Dec 07 '08 at 00:12
  • Good practice doesn't mean squat to me if it doesn't do what I need it to do. I'll eventually try to upgrade to a more graceful high-level solution for all of this when I can get my main problems solved. – Daddy Warbox Dec 07 '08 at 11:56

3 Answers3

6

Make the values WeakReferences (or SoftReferences) instead. That way the values can still be garbage collected. You'll still have entries in the map, of course - but you can periodically clear out the map of any entries where the Weak/SoftReference is now empty.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Damn, you beat me to the weak references :) – Morten Christiansen Dec 06 '08 at 21:14
  • 1
    Was busy editing - WeakHashMap isn't really suitable as it's the keys which end up being weak, not the values. You'll effectively have to write something somewhat similar, but it probably doesn't need to be quite as powerful if you're only using it for one particular situation. – Jon Skeet Dec 06 '08 at 21:15
  • Ok thanks. Yeah I was afraid of that. I'll probably just stick with using Remove methods and do it the lazy way then. :P – Daddy Warbox Dec 06 '08 at 21:25
5

Making an object visible to others before its constructor is complete is not thread safe.

It's not clear how the map is being used in this case, but suppose there's a static method like this in the class:

public static DataEntry getEntry(String name) {
  return _INTERNAL_LIST.get(name);
}

Another thread, running concurrently, can access a DataEntry while it is being created, and start to use an entry with an uninitialized VALUE. Even you reorder the code in the constructor so that adding the new instance to the map is the last thing you do, the JVM is allowed to reorder the instructions so that the object is added to the list first. Or, if the class is extended, the subclass initialization could take place after the object has been published.

If more than one thread accesses the interacts with the DataEntry class, you could have a concurrency bug that is platform dependent, intermittent, and very tough to diagnose.

The article, "Safe Construction," by Brian Goetz has more information on this topic.

Back to the original question: using WeakReference, as mentioned by others, is a good approach, but rather than iterating over every entry in the map, I'd recommend creating a wrapper for your values that extends WeakReference (it could be your DataEntry itself, or a helper), and queuing each reference in a ReferenceQueue. That way, you can quickly poll the queue for any collected entries, and remove them from the map. This could be done by a background thread (blocking on remove) started in a class initializer, or any stale entries could be cleaned (by polling) each time a new entry is added.

If your program is multi-threaded, you should abandon LinkedHashMap for a map from java.util.concurrent, or wrap the LinkedHashMap with Collections.synchronizedMap().

erickson
  • 265,237
  • 58
  • 395
  • 493
  • Thanks for the additional information. Multi-threading is a bridge I may eventually need to cross, but for now I'm just bumbling my way through the basics. Anyway I'll consider that alternate approach. – Daddy Warbox Dec 07 '08 at 12:05
1

What you want to use seems to be a weak reference. The concept is that weak references are not strong enough to force an object not to be GC'ed. I don't have much experience with them but you can learn more here.

Morten Christiansen
  • 19,002
  • 22
  • 69
  • 94