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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
