jsedding edited a comment on pull request #24: URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/24#issuecomment-743217153
@akankshajain18 I do not doubt for a second that sonar flags a _volatile_ field of a type other than a primitive or an enum as a bug. Primitive types and enums have in common that they are _immutable_, and that is what's important here. Even the Sonar rule's explanation talks about mutability: `For mutable objects, the volatile should be removed, and some other method should be used to ensure thread-safety [...]` However, we want to store an _immutable_ set in the _volatile_ field, which means even Sonar's explanation agrees that we're not actually in violation of their rule. Now, to add to the confusion, we would like our configuration update not only to be visible by multiple threads (all that the volatile keyword ensures is immediate visibility of the field's update to other threads), but we would also like the update to be _atomic_ (atomic, in case that's not clear, means that there are no intermediate (and possibly invalid) states visible during the update. Fortunately our configuration object (in this case a set of paths) can be _immutable_, if we wrap the set with `Collections.unmodifiableSet(Set)`. So we can create this immutable set and assign it _atomically_ to the _volatile_ field. This is perfectly good and correct Java. The fact that Sonar flags this as a bug is a bug in Sonar (or if you prefer a limitation). Java as a language does not have the concept of immutability built in like some other languages do. This makes it very hard for a static analysis tool like Sonar to determine when/if an object is or is not immutable. Owing to the complexity of this problem, it seems that Sonar chose to implement a very simple check instead of a correct one. Using a `CopyOnWriteArrayList` is IMHO not the correct solution for this problem. As I explained above, we are looking for `immutability` (because that's the characteristic of each state of configuration). However, `CopyOnWriteArrayList` is mutable. Furthermore, we are looking for _atomic_ updates. However, (assuming a _final_ field `CopyOnWriteArrayList`), updates of the `CopyOnWriteArrayList` are not _atomic_, because we would need to clear the previous state (or remove each element individually) and add the new elements. This is why I continue to advocate a solution which holds an immutable set in a volatile field. Sonar provides a way to ignore errors on a single line. I argue that this is a case where ignoring Sonar for a single line is warranted. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
