A DESCRIPTION OF THE PROBLEM :
Both java.beans.PropertyChangeSupport and java.beans.VetoableChangeSupport seem to have thread-safe implementations (though I truthfully am not 100% sure of that).  However, the documentation does not indicate either their thread-safety or lack thereof.  In keeping with the current documentation standards, this point should be indicated in the class documentation.  This will allow implementors to benefit from any thread-safety of the synchronization in the class implementations and allow proper multi-threaded implementation of these two classes.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
An indication of the classes thread-safety.
ACTUAL -
No indication.

URL OF FAULTY DOCUMENTATION :
http://java.sun.com/j2se/1.4.2/docs/api/java/beans/PropertyChangeSupport.html
(Incident Review ID: 246315) 
======================================================================
Posted Date : 2005-10-18 08:12:40.0

N/A

I took a quick peek at PropertyChangeSupport myself, out of curiosity. It appears as if the original intention may have been for this class to be thread-safe. However, I'm not sure if it has remained that way in its evolution. One item I am concerned about is how the firePropertyChange() method accesses the "listeners" and  "children" variables outside of any synchronization. I can't say for sure that this will cause a problem, but note that other code that creates and populates these variables is synchronized.

In contrast, VetoableChangeSupport wraps access to these variables in synchronized(this) in fireVetoableChange. I think this is the right thing to do, and ensures that the class is thread-safe. So, perhaps PropertyChangeSupport can be made to be more like VetoableChangeSupport and then we can document that they are thread-safe.

BTW, one more item of note. VetoableChangeSupport.fireVetoableChange() has the following lines of code:

java.util.Vector targets = null;
VetoableChangeSupport child = null;
synchronized (this) {
    if (listeners != null) {
        targets = (java.util.Vector) listeners.clone();
    }
    if (children != null && propertyName != null) {
        child = (VetoableChangeSupport)children.get(propertyName);
    }
}

if (listeners != null) {
    ....

If we're going to do the proper thing and synchronize around access to "listeners" (making a clone), then why don't we check that "targets" variable for null in the last line?
  xxxxx@xxxxx   2004-04-06

======================================================================
Posted Date : 2005-10-18 08:12:40.0

Contribution forum : https://jdk-collaboration.dev.java.net/servlets/ProjectForumMessageView?forumID=1463&messageID=15001
 (Three related bug-IDs: 4994635, 5026703, 6225071)
Posted Date : 2006-08-24 20:28:39.0

Just in case it might attract more internal attention: the reviewer above (xxxxx@xxxxx  2004-04-06) is right: this snippet is not thread-safe -- if listeners is accessed out-from-under you it may be surprising.  Unambiguously(*):  The last line should more probably be: if (targets != null) { ...  Though I didn't look at the rest of the code.  However, to boot, if that's the case then we're "being synchronized" for no benefit.  It should be thread-safe or else don't bother with the synchronized.  -- Not that I'm saying I have some kind of expectations...  All good.


*This style of writing must surely be attributed to JavaSoft.  I'll elucidate:

     Blah blah blah blah blah blah blah
blah blah.  Specifically:  Blah blah blah ...

It's this "Specifically" "clause" thing!  -- The above paragraph must most definitely seem like it came from a javadoc comment to you!  NO?

Can this please get updated.  After reviewing the actual source code for PropertyChangeSupport, I made the assumption that "it" is thread-safe.  Please update documentation and/or class so that gets classified as thread-safe

Let me rephrase my last comment:  Please update the source code and javadoc so that the class is thread-safe.