jsedding commented on a change in pull request #24:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/24#discussion_r524986775
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
##########
@@ -128,6 +129,9 @@
private volatile ResourceResolverFactoryConfig config = DEFAULT_CONFIG;
+ /** Alias path whitelist */
+ private AtomicReferenceArray<String> aliasPathAllowList;
Review comment:
The sonar message is misleading. A more accurate message would say that
a volatile field should only hold an immutable object (which includes
primitives). So wrapping a `List` using e.g. `Collections.unmodifiableList()`
would also be correct and _should not_ trigger any sonar warnings.
Alternatively, using a final field holding a thread-safe collection like
`CopyOnWriteArrayList` is also correct (in the sense of thread-safety).
Swapping out an immutable object with another in a volatile field
constitutes an atomic update, which is often desirable.
Updating a thread-safe collection is not necessarily atomic, so there
_could_ be a time-window when the field could be read and the collections
contents are in the middle of an update. This may or may not be a problem
depending on the use-case.
Using `Atomic*Array` sounds like bad advice to me, because it usually (my
opinion) you will not want to swap out the contents of a particular array index
atomically. It allows elements to be updated atomically, which is not what you
want here.
You want to swap out one configuration for another, so updating a volatile
field with an immutable collection sounds most correct to me.
----------------------------------------------------------------
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]
