Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Create rule to enforce that adaptable is not stored inside model #234

Open
kwin opened this issue Dec 19, 2022 · 2 comments
Open
Labels
feature rule New rule or modification of existing one.

Comments

@kwin
Copy link

kwin commented Dec 19, 2022

According to https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector the adaptable should not be stored in the model when cache=true is used, as this might exhaust the memory easily.
A rule to enforce that the adaptable is not stored in a field when the adaptable has cache=true should be added to prevent this.

@toniedzwiedz
Copy link
Collaborator

Hi @kwin, sorry for the late response, I missed it amidst the holiday/end-of-year craze. This does look like a good candidate for a rule.

I've got a couple of follow-up questions to better determine what the rule should look like.

  1. Is this only an issue when the @Self injector is used? Or is it enough to inject the underlying SlingHttpServletRequest?
  2. It seems the issue has been resolved apache/sling-org-apache-sling-models-impl@5b47255 so a rule would only be relevant to versions of Apache Sling that don't have the fix in place. Is this a correct interpretation?

@kwin
Copy link
Author

kwin commented Jan 17, 2023

  • Is this only an issue when the @Self injector is used? Or is it enough to inject the underlying SlingHttpServletRequest?

Any means to inject and store the adaptable in the model should be prevented. That also affects adaptable Resource (although to a lesser degree)

Although with that commit, heap space exhaustion is prevented, this still leads to unexpected behaviour. Compare with https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector

Starting in version 1.4.10, storing the original adaptable will not crash the JVM, but it can cause unexpected behavior (e.g. a model being created twice, when it should be cached). The issue was first reported in SLING-7586.

@toniedzwiedz toniedzwiedz added the rule New rule or modification of existing one. label May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature rule New rule or modification of existing one.
Projects
None yet
Development

No branches or pull requests

2 participants