[GitHub] [sling-org-apache-sling-models-impl] paul-bjorkstrand commented on a change in pull request #11: SLING-8069: allow non public constructors

[GitBox <gi...@apache.org>]

paul-bjorkstrand commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/11#discussion_r762436790



##########
File path: src/main/java/org/apache/sling/models/impl/model/ModelClassConstructor.java
##########
@@ -49,16 +50,49 @@ public ModelClassConstructor(Constructor<ModelType> constructor, StaticInjectAnn
         }
     }
 
-    public Constructor<ModelType> getConstructor() {
+    /**
+     * Proxies the call to {@link Constructor#newInstance(Object...)}, checking (and
+     * setting) accessibility first.
+     * 
+     * @param parameters
+     *            the constructor parameters that are passed to
+     *            {@link Constructor#newInstance(Object...)}
+     * @return The constructed object
+     * 
+     * @throws InstantiationException when {@link Constructor#newInstance(Object...)} would throw
+     * @throws IllegalAccessException when {@link Constructor#newInstance(Object...)} would throw
+     * @throws IllegalArgumentException when {@link Constructor#newInstance(Object...)} would throw
+     * @throws InvocationTargetException when {@link Constructor#newInstance(Object...)} would throw
+     * 
+     * @see Constructor#newInstance(Object...)
+     */
+    @SuppressWarnings({"java:S3011","java:S1874"})
+    public M newInstance(Object... parameters) throws InstantiationException, IllegalAccessException, IllegalArgumentException, InvocationTargetException {
+        synchronized (constructor) {

Review comment:
       You hit the nail on the head. I only put this here due to similar logic being in other, similar places around this repo. 
   
   The biggest reason I can think of for resetting it would be to ensure that other code, which may be restricted by a `SecurityManager` policy, can't take advantage of the field later being accessible.
   
   I wonder if this pattern should be revisited, and possibly removed. If the accessible wasn't "reset" after being set to true, then the synchronization would not be necessary, as every call would just `setAccessible(true)`. While there may be some risk involved in doing so, I think it will be minimal. The biggest risk I see would be with relation to access security checks via the SecurityManager. That being said, sometime after Java 17, security manager won't even exist, as it was [deprecated for removal][1]. Even in [Apache Felix][2], when `setAccessible(true)` is called, the accessible flag is not reset back to its previous value (good enough for them, then good enough for us?).
   
   During times of heavy model instantiation, especially in projects with heavy model reuse (e.g. a common parent class), the synchronized blocks could be causing some significant contention, causing a performance degradation. This is just a guess, and may need to be measured.
   
   [1]: https://openjdk.java.net/jeps/411
   [2]:https://github.com/apache/felix-dev/search?q=setAccessible




-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org