Hi Alfu Great to hear from you again in the Jackrabbit world! I am definitely looking forward to your contribution and active participation.
Sorting out Findbug issues is for sure a valuable way to become familiar with the code base as it forces you to dig into the details and read the code. You may also want to look at https://analysis.apache.org/dashboard/index/113395 In fact we never took the time to mark false positive issues or issues we intentionally never fixed... that might also be something to get started (to be honest I just don't know how to mark the issues that I looked at and considered false positives). You will also spot that the Sonar complains about unused fields or methods, which are there for OSGi integration but somehow don't get properly recognised by the analysis. But nonetheless there is for sure plenty of room for improvement in all parts of Oak and sufficient stuff to get your hands dirty. Kind regards Angela On 20/04/16 15:54, "Alfusainey Jallow" <[email protected]> wrote: >Hi, > >I am currently going through the oak code base trying to address issues >reported by the findbugs analysis tool[1], some of which i try to address >in OAK-4257[2]. I did not want touch some of the concurrency issues >reported without first writing to the list. > >In DocumentNodeStore (inside of oak-core plugins), syncronization is >performed on the 'isDisposed' instance field which is declared as >AtomicBoolean. now in the DocumentNodeStore#dispose() call, you have this: > >// notify background threads waiting on isDisposed >synchronized (isDisposed) { > isDisposed.notifyAll(); >} > > >Findbugs is warning about this saying: > >This method performs synchronization an object that is an instance of a >> class from the java.util.concurrent package (or its subclasses). >>Instances >> of these classes have their own concurrency control mechanisms that are >> orthogonal to the synchronization provided by the Java keyword >> synchronized. For example, synchronizing on an AtomicBoolean will not >> prevent other threads from modifying the AtomicBoolean. > > > >Such code may be correct, but should be carefully reviewed and documented, >> and may confuse people who have to maintain the code at a later date. > > >since i am not yet familiar with the code base, i decided to raise this >before attempting to fix it (synchronizing on *this* object instead?) or >may be this is intentional? the same issue is >in DocumentNodeStore.NodeStoreTask#run() call. > >Most NPEs issues (possible null pointer derefence) are also reported in >many parts of the code base and most complains results in a call to >NodeBuilder#getProperty(String). this method can return null and the >return >object should always be checked for null. however, most of its usage in >the >code base does not explicitly check for a null-return value. is this >intentional too? > >[1] https://analysis.apache.org/dashboard/index/113395 >[2] https://issues.apache.org/jira/browse/OAK-4257 > >kind regards >-- >alfu
