Safe construction techniques
Don't publish the "this" reference during construction
public class EventListener {
public EventListener(EventSource eventSource) {
// do our initialization
...
// register ourselves with the event source
eventSource.registerListener(this);
}
}
Even ignoring all the Java memory Model issues such as differences in visibility accross threads and memory access reording, this code still is in danger of exposing an incompletely constructed EventListener object to other threads. Consider what happens when EventListener is subclassed, as below:
public class RecordingEventListener extends EventListener {
private final ArrayList list;
public RecordingEventListener(EventSource eventSource) {
super(eventSource);
list = Collections.synchronizedList(new ArrayList());
}
public onEvent(Event e) {
list.add(e);
super.onEvent(e);
}
}
If the event source decides to send an event from within the registration call,
or we just get unlucky and an event arrives at exactly the wrong moment,
RecordingEventListener.onEvent()
could get called while list still has
the default value of null.
Don't implicitly expose the "this" reference
public class EventListener2 {
public EventListener2(EventSource eventSource) {
eventSource.registerListener(
new EventListener() {
public void onEvent(Event e) {
eventReceived(e);
}
}
)
}
public void eventReceived(Event e) {}
}
Non-static inner classes maintain an implicit copy of the this
reference of
their parent object, so creating an anonymous inner class instance and passing it
to an object visible from outside the current thread has all the same risks as
exposing the this
reference itself.
Don't start threads from within constructors
Actually I'm get confusing on this, my question is that is what if we want to have
an instance field that points to a class that start threads within constructors?
or if the class provides a start()
method, is it ok for us to call the
start()
method in our constructor. if the newly started threads need to
access our object, then i think it not ok, since our object is not fully constructed
yet.
What does "publish" mean?
public class Safe {
private Object me;
private Set set = new HashSet();
private Thread thread;
public Safe() {
// Safe because "me" is not visible from any other thread
me = this;
// Safe because "set" is not visible from any other thread
set.add(this);
// Safe because MyThread won't start until construction is complete
// and the constructor doesn't publish the reference
thread = new MyThread(this);
}
public void start() {
thread.start();
}
private class MyThread(Object o) {
private Object theObject;
public MyThread(Object o) {
this.theObject = o;
}
...
}
}
public class Unsafe {
public static Unsafe anInstance;
public static Set set = new HashSet();
private Set mySet = new HashSet();
public Unsafe() {
// Unsafe because anInstance is globally visible
anInstance = this;
// Unsafe because SomeOtherClass.anInstance is globally visible
SomeOtherClass.anInstance = this;
// Unsafe because SomeOtherClass might save the "this" reference
// where another thread could see it
SomeOtherClass.registerObject(this);
// Unsafe because set is globally visible
set.add(this);
// Unsafe because we are publishing a reference to mySet
mySet.add(this);
SomeOtherClass.someMethod(mySet);
// Unsafe because the "this" object will be visible from the new
// thread before the constructor completes
thread = new MyThread(this);
thread.start();
}
public Unsafe(Collection c) {
// Unsafe because "c" may be visible from other threads
c.add(this);
}
}
Conclusion
Try to avoid using this, Try to avoid creating instances of inner classes or
starting threads from constructors. If you cannot avoid using this, be very
sure that you are not making the this
reference visible to other threads.