Opened 47 years ago

Last modified 7 years ago

#889 new task

IZ572: Evaluate all equals/hashCode methods

Reported by: afisch Owned by:
Priority: high Milestone:
Component: hedeby Version: 1.0u1
Severity: Keywords: misc
Cc:

Description

[Imported from gridengine issuezilla http://gridengine.sunsource.net/issues/show_bug.cgi?id=572]

        Issue #:      572          Platform:     All         Reporter: afisch (afisch)
       Component:     hedeby          OS:        All
     Subcomponent:    misc         Version:      1.0u1          CC:    None defined
        Status:       NEW          Priority:     P2
      Resolution:                 Issue type:    TASK
                               Target milestone: 1.0u5next
      Assigned to:    adoerr (adoerr)
      QA Contact:     adoerr
          URL:
       * Summary:     Evaluate all equals/hashCode methods
   Status whiteboard:
      Attachments:
                      Date/filename:                                   Description:              Submitted by:
                      Tue Sep 30 00:47:00 -0700 2008: review_571_2.txt Review paper (text/plain) rhierlmeier


     Issue 572 blocks:
   Votes for issue 572:     Vote for this issue


   Opened: Tue Sep 9 05:49:00 -0700 2008 
------------------------


   Description:

   There are several classes that have an overridden equals/hashCode method. Three
   problems are should be addressed with this task that are related to these methods:

   1. The equals / hashCode contract
   equals has to fulfill identity (a.equals(a)==true), symmetry
   (a.equals(b)==b.equals(a)) and transitivity (a.equals(b) && b.equals(c)==>
   a.equals(c)) If equals is overridden then hashCode has to be overridden, too.
   The hashCode contract requires that if two objects are equal in the sense of the
   equals() method, they get the same hashCode. This does hold the other way round!
       As the hashCode of any object has to be constant over its lifetime. This
   implies that for any reasonable hashCode implementation that the equals
   relationship between two objects has to remain constant over their lifetime,
   too.

   2. Guideline to implement equals and hashCode

   The equals method considers the state of the member variables for the equals
   determination. These members can have a set of properties that have to be
   considered first:
     a.) primitive members
     b.) members that are never null
     c.) members that might be null
     d.) transient members
     e.) mutability
   These properties allow to determine the scope of the equals method. The scope
   includes all member variables that are relevant for the compare. Normally all
   members are respected in the equals method. Primitive members and members that
   are never null are easy to compare in general. But the case of default
   initialization of the members should be considered. Members that might be null
   have to be considered with respect to the null case. Transient members have to
   be treated with special care as they might not be relevant for the compare.
   Mutability usually has implications for the hashCode calculation (Usually
   mutability implies the same constant hash code for all objects to not violate
   the hashCode contract).

   The equals method should be implemented in an uniform way. This ensures that it
   is easy to review / maintain. The reference below explain a strategy in greater
   detail:
     Effective Java™, Second Edition, Joshua Bloch, Prentice Hall,ISBN PRINT
   0-321-35668-3,eText ISBN 0-13-715002-4 Chapter 3:Item 8 & 9
     (The book does not address the hashcodes for mutable object)
     http://www.vipan.com/htdocs/hashCode_help.html
     http://www.ibm.com/developerworks/java/library/j-jtp05273.html
   The suggested way the equals /hashCode method should be implemented would
   address the above problem.

   3.) Testing. If an equals method is implemented it should be tested first with
   respect to the equals / hashCode contract and then to specific aspects like
   equivalence of a set of subclasses.



   Evaluation:
   The problem is considered a P2 Task as it is critical to have correct
   implementations of the equals /hashCode methods to avoid memory leaks in
   Hashtables and other unexpected system behavior.



   Suggested Fix/Work Around:
   Reconsider all equals /hashCode methods whether they are conform with the above
   requirements



   Analysis:

   All hashCode / equals methods are analyzed for the following details:
   if one is there: are both there, hashCode and equals?
   mutable? if yes ==> hashCode constant? no ==> all involved members final
   all members considered?
   same members considered for hashCode and equals?
   can equals / hashCode be improved by using uniform way of writing it?
   is equals / hashCode correct? (equals/hashCode contract fulfilled? / all cases
   of comparison considered?)
   is hashCode cached?



   ComponentInfo 248
     no hash caching

   ExecutionEnvImpl.java 192
     csInfo is mutable and part of equals() but hashCode is not constant!
     csInfo can be null but is not checked!
     equals could be rewritten in uniform fashon

   JvmInfo 82
     The hostname object is currently a mutable object and returns a constant value
   for hashCode! It has to be used instead of the member hostname  ==> this makes
   the member hostname redundant.
     strange hash caching (h<0) // leads to recalculation for any negative hash
   value! (eg. in the case of integer overflow!)

   AbstractConfigurationServiceEvent.java 157
     Members are not checked for null before compare (except csName in constructor)
     no hash caching

   AbstractComponentEvent.java 123
     Not uniform hashing (  hostname.hashCode() is no separate line)
     equals could be rewritten in uniform fashon

   ComponentOperationDescriptor.java
     OK (minor: no @override)

   HostResourceId.java 85 & OsRessourceId.java 90 &
   CandidateHostResourcePropertiesId 98
     else if (obj instanceof String) { violates symmetry contract of equals!
     has to be removed with care, watch for dependencies!
     hostname/osname/id should be declared final
     equals could be rewritten in uniform fashon
     no hash caching

   AbstractRequest.java 114
     equals is implemented wrong! no else conditions!
     serviceName and sloName should be final
     some members (needs, state) are not considered! ok?
     equals could be rewritten in uniform fashon
     no hash caching

   AbstractResourceId.java
     dangerous construct! concrete hashCode impl but abstract equals! ==> does not
   make sense!
     both abstract or both implemented ==> has lead allready to problems in
   subclasses!! Consider AbstactManagementEvent as a better way to implement this!
   Be aware that HostResourceImpl is mutable (see below)
     BTW: serialVersionUID seems to be not needed as class is abstract

   AnyResourceIdImpl.java 82
     The implementation has no hashCode method this method is implemented in the
   superclass ==> why?
     It allows compare with ResourceId that may violate symmetry!

   HostResourceImpl.java.99
     getId is the only thing compared in equals. It gives back Hostname string ==>
   but hostname is mutable!! ==> as getId is used by superclass for hashCode this
   leads to hashCode condtract violation!! hashCode has to be reimplemented here
   and return constant value as Hostname objects are compared!
     As Hostname is mutable, HostResourceImpl is mutable ==> ResourceId objects
   that are mutable! ==> hashCode has to be constant for all classes that inplement
   ResourceId and might return true if compared with Hostname!
     However equals may violate symmetry contract!! as it is allowed to compare
   with ResourceId objects,too.

   TempResourceId.java 116
     TempResourceId is a candidate that can violate the symmetry contract!
     Consider the following scenario with h a HostResourceImpl and b TempResourceId
   that wraps a. a.equals(b) will return true. As b is a ResourceId and a.id ==
   b.getDelegate().id; But b.equals(a) will return false as a is not a
   TempResourceId!
     BTW: unwrapTemporaryResourceId does exactly the opposite as one would expect
   from the name!

   Order 226
     Wired compare in equals: return (this == null);
     Unefficient compare (all compares are made although retVal might be allready
   false): equals could be rewritten in uniform fashion
     Type and Target are not compared, but used in hashCode !!! ==> violation of
   contract! Different hashCode for equal objects!
     no hash caching

   RequestImpl.java 111
     hashCode impl does not match equals method less members are used!! This does
   not mean a violation of the hash code contract as equal objects can not have
   different codes but the hashCode might perform poorly.
     equals method is implemented wrong, no else case considered! It could be
   rewritten in uniform fashon
   RequestImpl.java:NormalizedUrgency 766
     OK: equals could be rewritten to for performance purpose (all compares are
   done independent of false intermediate compares!), hashCode: first (97 * h) is
   allways 0 and therefore useless.

   ResourceImpl.java 342
     equals can compare ResourceImpl objects with ResourceId objects, but
   ResourceImpl is not a ResourceId==> violation of symmetry contract!!!!
     equals and hashCode do not compare type and properties. Is this ok?
     Warning: The id field can be a HostResourceImpl object that is dependent on
   Hostname which is mutable!! ==> ResourceImpl is mutable==> hashCode impl !must
   not! rely on some id.getId().hashCode()!!!

   AbstactManagementEvent 107:
     is Abstract but implements equals and hashCode! ==> final methods?
     (all subclasses do not overwrite equals and hashCode!)
     no hash caching implemented!
     BTW: all subclasses override toString ==> abstract public String toString()?
     BTW: serialVersionUID seems to be not needed as class is abstract

   PriorityPolicy 123:
     Warning super class member name is not part of the compare! Ok?
     no hash caching implemented!

   RolePrincipal 62:
     No check for member name if null in Constructor, equals and hashCode!
     no hash caching implemented!

   AbstractServiceEvent 107:
      is Abstract but implements equals and hashCode! ==> final methods?
     (all subclasses do not overwrite equals and hashCode!)
     no hash caching implemented!
     BTW: all subclasses override toString ==> abstract public String toString()?
     BTW: serialVersionUID seems to be not needed as class is abstract

   AbstractSLOManagerEvent 97:
    is Abstract but implements equals and hashCode! ==> final methods?
     (all subclasses do not overwrite equals and hashCode!)
     no hash caching implemented!
     BTW: all subclasses override toString ==> abstract public String toString()?
     BTW: serialVersionUID seems to be not needed as class is abstract

   Usage 77: OK

   CommandPermission 162:
     no hash caching implemented!

   GrmClassLoaderFactory:ClassLoaderElem 239
     Why is the hashCode impl part of the constructor ==> should go to hashCode?
     parent compare could be rewritten in uniform fashion

   HostAndPort 181:
     host compare & hashing could be rewritten in uniform fashion
     no hash caching implemented!

   GrmException 104:
     equals is static why not object instance? ==> rename to
   areFirstStackTraceElementEqual()
     equals does not compare for null (it should be not necessary but who knows!)
     No corresponding hashCode implemented if equals is considered to become a real
   equals!!
     BTW correct place for static method? as general Exceptions are compared and
   not GrmExceptions?

   HostImpl 1021:
     OK
     Warning: HostImpl objects are a mutable as they depend on Hostname objects!
   equals and hashCode should not be changed!
     BTW: Class has public constructor although it has static newInstance methods
   ==> Ok?


   DelegatingLoginModule:CacheElem 350:
     Why is the hashCode impl part of the constructor ==> should go to hashCode?
     parent compare could be rewritten in uniform fashion

   UserPrincipal 99:
     Ok
     no hash caching implemented!

   RolesUtil: 77 /114 /134
     equal Methods for Role and Principal should be implemented in corresponding
   classes. (dont forget hashCode then!)
     equal Method for list should be renamed to equalsIgnoreOrder().
     Although these equal method are not real equal methods yet they can gain
   readability/maintainability! by being rewritten in uniform fashion!
     BTW: for(Role tr: rl2) { in line 148 can be committed if contains() is used!
     BTW: for(Principal tmpp: r2.getPrincipal()) { in line 118, too.

   GrmDaemonPrincipal:
     OK!
     no hash caching implemented!
     Forgotten: Override Annotations

   General Comments:
   The compare instanceof allows a widening of the object range considered by
   equals via subclassing. This can potentially violate the symmetry contract
   (myDog instanceOf Mammal ==> but *NOT* myMammal instance of Dog). It should be
   discussed if we use this.getClass().equals(other.getClass()). The "effective
   java" book states that the getClass() variant may violate Liskovs substitute
   principle. Composition should be used instead of inheritance if a subclass would
   extend the set of members.

   should a member variable to cache a hashCode generally be transient?
   Recalculation versus  the serialization overhead? Should every hash variable be
   volatile (threadsafe) suggestion from "effective Java"?

   A nice way to implement the equals /hashCode method in an uniform fashion would
   be similar to the one suggested in:
     http://www.ibm.com/developerworks/java/library/j-jtp05273.html

   Please take a look at the Hostname class it has been changed to bugfix iZ#508
   and as it is a mutable object, the hashCode is now a constant!


   How to test:
   There should be tests for the equals and cashCode contract. A general frame work
   could be extracted from HostnameTest class (part of bugfix for 508).

   There should be a test for all kind of  objects that implement ResourceId that
   are checked vice versa for equals symmetry! And if they are conform with the
   hashCode contract! A problem here is, that HostResourceImpl is mutable! ==>
   hashCodes should be the same for any ResourceId object!!!
   All cases (AbstactManagementEvent,AbstractServiceEvent,AbstractSLOManagerEvent)
   where the equals and hashCode are inhereted should be checked in a similar manner.

   Before this issue is started. The Hostname class should be discussed. The fact
   that the Hostname class is implemented as a mutable object leads to a lot of
   ugly side effects. As the hashCode for Hostname is constant, Hostname objects
   will perform poorly as keys in any Hashtable/Map, although they are often used
   for this purpose!

   ETC:
   5 PD
               ------- Additional comments from afisch Fri Sep 12 02:22:32 -0700 2008 -------
   A set of three methods to test equals() /hashCode() methods  are now available in :
     com.sun.grid.grm.TestUtil

   A detailed Example how to use them can be found in:

     com.sun.grid.grm.util.HostnameTest.testEqualsAndHashCode()


               ------- Additional comments from rhierlmeier Tue Sep 30 00:47:24 -0700 2008 -------
   Created an attachment (id=26)
   Review paper

               ------- Additional comments from rhierlmeier Tue Sep 30 01:29:55 -0700 2008 -------
   I attached by mistake the review paper of issue 571 to issue 572. Unfortunately
   issuezilla can not delete attachment.

   Sorry for that.
               ------- Additional comments from rhierlmeier Wed Nov 25 07:21:12 -0700 2009 -------
   Milestone changed

Change History (0)

Note: See TracTickets for help on using tickets.