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

Qute: fix possible stack overflow error in InsertSectionHelper #41517

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Jun 27, 2024

@quarkus-bot quarkus-bot bot added the area/qute The template engine label Jun 27, 2024
@mkouba
Copy link
Contributor Author

mkouba commented Jun 27, 2024

@gbourant @ia3andy This fix is suboptimal in terms of performance - I ran the simple microbenchmark for {#include} and the regression is ~ 20%. Unfortunately, I don't see a better way with the current API and I'm not sure how to enhance the API to make it better.

UPDATE: I think I found a way to mitigate the perf impact. I will update the PR tomorrow.

@gbourant
Copy link

@mkouba, I don't know if this is possible but at build time you might be able to give a random name to the insert directive and wrap the content of other templates to that named directive. (maybe it should be enabled via config?)

For example the following templates:

root.html

<html>
<body>{#insert /}</body>
</html>

login.html

{#include auth.html}
    <form>Login Form</form>
{/include}

become:

root.html

<html>
<body>{#insert someRandomName /}</body>
</html>

login.html

{#include auth.html}
    {#someRandomName}
    <form>Login Form</form>
    {/}
{/include}

@mkouba
Copy link
Contributor Author

mkouba commented Jun 28, 2024

@mkouba, I don't know if this is possible but at build time you might be able to give a random name to the insert directive and wrap the content of other templates to that named directive. (maybe it should be enabled via config?)

...

The problem is we have the same problem if those nested includes use the same explicit block name, i.e. {#insert foo /}. So the fix has to be a bit more robust.

@mkouba mkouba marked this pull request as ready for review June 28, 2024 06:50
@gbourant
Copy link

I just tried it and it works buddy! :)

@mkouba
Copy link
Contributor Author

mkouba commented Jun 28, 2024

I just tried it and it works buddy! :)

What exactly? The fix in this PR? ;-)

@gbourant
Copy link

I just tried it and it works buddy! :)

What exactly? The fix in this PR? ;-)

Yeah the fix in this PR. It doesn't throw OverflowError when using nested {#insert /}

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

LGTM thanks!!

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 28, 2024
@ia3andy ia3andy merged commit c24efa8 into quarkusio:main Jun 28, 2024
62 of 65 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 28, 2024
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jun 28, 2024
@ia3andy
Copy link
Contributor

ia3andy commented Jul 1, 2024

@mkouba maybe we should backport this furtther?

@mkouba
Copy link
Contributor Author

mkouba commented Jul 1, 2024

@mkouba maybe we should backport this furtther?

Yes, it's already labeled with triage/backport...

@ia3andy
Copy link
Contributor

ia3andy commented Jul 1, 2024

@mkouba triage backport is just for 3.12, but we should backport it to as far as it's compatible no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Qute: StackOverflowError due to nested #include and #insert directives
5 participants