Skip to content

Fix OutOfMemoryError in reference component #186

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

Open
wants to merge 8 commits into
base: develop-sling12
Choose a base branch
from

Conversation

reggie7
Copy link
Contributor

@reggie7 reggie7 commented Mar 30, 2021

Fix for #163, but I had to use #184 fix for #164. The core fix is this commit.

@reggie7 reggie7 requested review from reusr1, a user, GastonGonzalez, cmrockwell and flx-sta March 30, 2021 16:40
@reggie7 reggie7 self-assigned this Mar 30, 2021
Copy link
Collaborator

@flx-sta flx-sta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to fix the backend-part of the issue 🚀 .
@reggie7 please have a look at my question.

Im not sure though why the frontend even allows to reference a page to itself.
Is there an issue to fix this? that it shouldn't be allowed in the first place

@reggie7
Copy link
Contributor Author

reggie7 commented Apr 1, 2021

Im not sure though why the frontend even allows to reference a page to itself.
Is there an issue to fix this? that it shouldn't be allowed in the first place

@Felix-Puetz hmm, we can discuss that. What do you think @reusr1?
It's not that easy, though, as just an assumption that same page can't be referenced only. There's more to it. We can't allow any loop of references actually. This requires a call to the server so that it can check it properly.

@reusr1
Copy link
Contributor

reusr1 commented Apr 3, 2021

@Felix-Puetz I think a ref to the page itself is ok
@reggie7 looking at this PR I am a bit worried about the performance implication of this code, have to think about this a bit more

Copy link
Contributor

@reusr1 reusr1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently a no, need to figure out if this change has a performance implication/if there is no cleaner way to handle this

@reusr1
Copy link
Contributor

reusr1 commented Apr 3, 2021

@reggie7 can we not solve this much more performant with the introduction of a threadlocal variable at

ResourceResolver resourceResolver = getResource().getResourceResolver();
that makes sure we dont output the same resource multiple times?

@reggie7
Copy link
Contributor Author

reggie7 commented Apr 7, 2021

@Felix-Puetz I think a ref to the page itself is ok
@reggie7 looking at this PR I am a bit worried about the performance implication of this code, have to think about this a bit more

Sure, take your time, it is indeed doing a recursive search to avoid recursive includes.

@reusr1
Copy link
Contributor

reusr1 commented Apr 7, 2021

@Felix-Puetz I think a ref to the page itself is ok
@reggie7 looking at this PR I am a bit worried about the performance implication of this code, have to think about this a bit more

Sure, take your time, it is indeed doing a recursive search to avoid recursive includes.

@reggie7 I think you missed my last comment - I am suggesting to use a threadlocal variable to handle this instead

@reggie7 reggie7 requested a review from reusr1 April 14, 2021 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants