Hi Angela, yes, sure! Thanks much for that. i will definitely participate in the oak effort and i want to do it with fun. i only hope that you will not ask me to slow down with patches once i settle down with the code base, since they will only keep coming;-).
the next step will be to look into the OAK-3626[0] new feature for the auth-ldap module. i think this will require quiet some effort.. so i want to start by proposing a draft for how it could look then rework it (base on received feedback) and so until something concrete can be done. i will be adding specific comments on the issue ticket instead You can have a look at the patch for the ' CompositeAuthorizationConfiguration', particularly the modified if check. The rest is clear, especially with the DocumentNodeStore. I also see that a false alarm can be turn off in findbug using the @SuppressFBWarnings annotation[1]. maybe this or something similar can be used to reduce the false alarms? [0] https://issues.apache.org/jira/browse/OAK-3626 [1] http://jmri.sourceforge.net/help/en/html/doc/Technical/FindBugs.shtml Kind regards 2016-04-20 15:29 GMT+00:00 Angela Schreiber <[email protected]>: > 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 > > -- alfu
