You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2020/10/16 08:30:36 UTC
[GitHub] [sling-org-apache-sling-models-impl] cjelger opened a new pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
cjelger opened a new pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21
- use ServletRequest attributes to cache sling models when the adaptable is a ServletRequest
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] sonarcloud[bot] commented on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-712794668
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SEC
URITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='93.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list) [93.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] sonarcloud[bot] commented on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-712211298
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/C.png' alt='C' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SEC
URITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list) [94.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger commented on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
cjelger commented on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-709911017
@justinedelson @cziegeler @raducotescu Can you please review the PR? Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] raducotescu commented on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
raducotescu commented on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-712740266
SonarCube is right and you have the rule's explanation: "The story is very similar for synchronizing on parameters; two different threads running the method in parallel could pass two different object instances in to the method as parameters, completely undermining the synchronization."
Usually you should synchronize on objects you control. For the case where the `adaptable` is a request you don't need to do any synchronization. For the other case I think it's sufficient to use `adapterCache.computeIfAbsent`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] sonarcloud[bot] commented on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-712196068
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/C.png' alt='C' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [3 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SEC
URITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100.png' alt='100.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] raducotescu commented on a change in pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
raducotescu commented on a change in pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#discussion_r507719558
##########
File path: src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
##########
@@ -404,11 +430,7 @@ public boolean isModelClass(@NotNull Class<?> type) {
ModelType model = (ModelType) Proxy.newProxyInstance(modelClass.getType().getClassLoader(), new Class<?>[] { modelClass.getType() }, handlerResult.getValue());
if (modelAnnotation.cache()) {
- Map<Class<?>, SoftReference<Object>> adaptableCache = adapterCache.get(cacheKey);
- if (adaptableCache == null) {
- adaptableCache = Collections.synchronizedMap(new WeakHashMap<Class<?>, SoftReference<Object>>());
- adapterCache.put(cacheKey, adaptableCache);
- }
+ Map<Class<?>, SoftReference<Object>> adaptableCache = getOrCreateCache(adaptable);
Review comment:
I think my question was not clear enough. I'm saying that if the `modelAnnotation#cache` returns `true`, we can already create the cache for the `adaptable` at https://github.com/apache/sling-org-apache-sling-models-impl/pull/21/files#diff-0c273224a9fa6e263f4381c5d4b8145aa2c6c89e75e0b62025eea5efa17edec9R401, using `getOrCreate`.
Further down, you just need to populate the cache. I think the `getCache` and `getOrCreate` should be merged and make the code simpler to read.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] sonarcloud[bot] commented on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-709909993
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SEC
URITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='80.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list) [80.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] raducotescu merged pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
raducotescu merged pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger commented on a change in pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
cjelger commented on a change in pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#discussion_r507677579
##########
File path: src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
##########
@@ -404,11 +430,7 @@ public boolean isModelClass(@NotNull Class<?> type) {
ModelType model = (ModelType) Proxy.newProxyInstance(modelClass.getType().getClassLoader(), new Class<?>[] { modelClass.getType() }, handlerResult.getValue());
if (modelAnnotation.cache()) {
- Map<Class<?>, SoftReference<Object>> adaptableCache = adapterCache.get(cacheKey);
- if (adaptableCache == null) {
- adaptableCache = Collections.synchronizedMap(new WeakHashMap<Class<?>, SoftReference<Object>>());
- adapterCache.put(cacheKey, adaptableCache);
- }
+ Map<Class<?>, SoftReference<Object>> adaptableCache = getOrCreateCache(adaptable);
Review comment:
No there could be a cache with the same `adaptable` but a different "target" model class. For example, one could first `adapt` the request to `MyModel.class` and then to `MyOtherModel.class`. So in the first case, the cache doesn't exist and the model is of course not cached, but in the 2nd case, the cache exists but the model is also not cached.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger commented on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
cjelger commented on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-712798084
@raducotescu Just fixed the last issue, thx for your feedback.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger commented on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
cjelger commented on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-712216861
@cziegeler @raducotescu I included your PR feedback: keep one single method to get or create the cache, and `synchronized` it on the adaptable to make sure we don't get any race condition. However SunarQube complains about the `synchronized` block because it's done on a method parameter, although here we exactly want to synchronize that block **_per_** adaptable. Not sure if this is a strict check ad I should fix or not. WDYT?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] sonarcloud[bot] removed a comment on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-712211298
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/C.png' alt='C' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SEC
URITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list) [94.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] raducotescu commented on a change in pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
raducotescu commented on a change in pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#discussion_r507661762
##########
File path: src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
##########
@@ -404,11 +430,7 @@ public boolean isModelClass(@NotNull Class<?> type) {
ModelType model = (ModelType) Proxy.newProxyInstance(modelClass.getType().getClassLoader(), new Class<?>[] { modelClass.getType() }, handlerResult.getValue());
if (modelAnnotation.cache()) {
- Map<Class<?>, SoftReference<Object>> adaptableCache = adapterCache.get(cacheKey);
- if (adaptableCache == null) {
- adaptableCache = Collections.synchronizedMap(new WeakHashMap<Class<?>, SoftReference<Object>>());
- adapterCache.put(cacheKey, adaptableCache);
- }
+ Map<Class<?>, SoftReference<Object>> adaptableCache = getOrCreateCache(adaptable);
Review comment:
Don't we already know here that there was no cache created for this `adaptable`? If a cache was available, we would have returned the result already - see https://github.com/apache/sling-org-apache-sling-models-impl/pull/21/files#diff-0c273224a9fa6e263f4381c5d4b8145aa2c6c89e75e0b62025eea5efa17edec9R401.
##########
File path: src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
##########
@@ -421,11 +443,7 @@ public boolean isModelClass(@NotNull Class<?> type) {
result = createObject(adaptable, modelClass);
if (result.wasSuccessful() && modelAnnotation.cache()) {
- Map<Class<?>, SoftReference<Object>> adaptableCache = adapterCache.get(cacheKey);
- if (adaptableCache == null) {
- adaptableCache = Collections.synchronizedMap(new WeakHashMap<Class<?>, SoftReference<Object>>());
- adapterCache.put(cacheKey, adaptableCache);
- }
+ Map<Class<?>, SoftReference<Object>> adaptableCache = getOrCreateCache(adaptable);
Review comment:
Don't we already know here that there was no cache created for this `adaptable`? If a cache was available, we would have returned the result already - see https://github.com/apache/sling-org-apache-sling-models-impl/pull/21/files#diff-0c273224a9fa6e263f4381c5d4b8145aa2c6c89e75e0b62025eea5efa17edec9R401.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] sonarcloud[bot] removed a comment on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-709909993
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SEC
URITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='80.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list) [80.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger commented on a change in pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
cjelger commented on a change in pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#discussion_r507682094
##########
File path: src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
##########
@@ -404,11 +430,7 @@ public boolean isModelClass(@NotNull Class<?> type) {
ModelType model = (ModelType) Proxy.newProxyInstance(modelClass.getType().getClassLoader(), new Class<?>[] { modelClass.getType() }, handlerResult.getValue());
if (modelAnnotation.cache()) {
- Map<Class<?>, SoftReference<Object>> adaptableCache = adapterCache.get(cacheKey);
- if (adaptableCache == null) {
- adaptableCache = Collections.synchronizedMap(new WeakHashMap<Class<?>, SoftReference<Object>>());
- adapterCache.put(cacheKey, adaptableCache);
- }
+ Map<Class<?>, SoftReference<Object>> adaptableCache = getOrCreateCache(adaptable);
Review comment:
No, the same adaptable can be adapted to multiple types. For example, the request could be adapted first to `MyFirstModel.class` and then to `MySecondModel.class`: so in the first case there is no cache and of course no cached model, but in the 2nd case, the cache exists but doesn't contain a cached model for the `MySecondModel.class` (but would contain a cached model for `MyFirstModel.class`).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] sonarcloud[bot] removed a comment on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-712196068
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/C.png' alt='C' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG) [3 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SEC
URITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100.png' alt='100.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=21&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger edited a comment on pull request #21: SLING-9834 - [Sling Models] Caching bug with reused Servlet requests
Posted by GitBox <gi...@apache.org>.
cjelger edited a comment on pull request #21:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/21#issuecomment-712216861
@cziegeler @raducotescu I included your PR feedback: keep one single method to get or create the cache, and `synchronized` it on the adaptable to make sure we don't get any race condition. However SunarQube complains about the `synchronized` block because it's done on a method parameter, although here we exactly want to synchronize that block **_per_** adaptable and it's declared `final`. Not sure if this is a strict check and I should fix or not. WDYT?
----------------------------------------------------------------
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:
users@infra.apache.org