Skip to content

Commit Session on Include Dispatch calls commitSession each time #2285

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

Closed

Conversation

laurentschoelens
Copy link

This provides a fix proposal for #2284

It marks the current request that a first-commit has been done in a include context to not commit on every requestDispatcher.include call

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 30, 2023
@laurentschoelens
Copy link
Author

Tested successfully on my app.

Before fix, JFR results :
image

After fix, JFR results :
image

@laurentschoelens
Copy link
Author

I've not been able to run all tests on my laptop and not found information on how to do it on documentation.

@marcusdacoregio marcusdacoregio self-assigned this Jun 12, 2023
@marcusdacoregio marcusdacoregio added status: duplicate A duplicate of another issue type: enhancement A general enhancement in: core and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 12, 2023
@marcusdacoregio marcusdacoregio added this to the 3.2.0-M1 milestone Jun 12, 2023
@marcusdacoregio
Copy link
Contributor

Hi @laurentschoelens, we are gonna consider this as an enhancement since the code is running as expected and this is a performance enhancement. Can you rebase your branch off of main and also add a test to make sure the session is not committed again? Ideally, the test should fail without your changes and pass with them.

@laurentschoelens laurentschoelens changed the base branch from 2.6.x to main June 19, 2023 13:51
@laurentschoelens
Copy link
Author

laurentschoelens commented Jun 19, 2023

Hi @marcusdacoregio : hope done well :)
I've added one unit test that fails with actual code and pass with my changes.
Since SpringBoot 2.7 is still in maintenance, could I hope to have it backported to latest 2.X branch ?

@marcusdacoregio marcusdacoregio removed the status: duplicate A duplicate of another issue label Jun 23, 2023
@marcusdacoregio marcusdacoregio changed the title [GH-2284] fix commitSession() called multiple times in requestDispatcher.include Commit Session on Include Dispatch calls commitSession each time Jun 23, 2023
@laurentschoelens
Copy link
Author

@marcusdacoregio seen that build failed due to checkstyle violation on my new unit test 😔

@marcusdacoregio
Copy link
Contributor

Don't worry @laurentschoelens, I can take care of it before merging

marcusdacoregio added a commit that referenced this pull request Jun 23, 2023
@marcusdacoregio
Copy link
Contributor

Thanks @laurentschoelens, this is now merged into main via 7360723 with this polish commit 2d43c31

marcusdacoregio added a commit that referenced this pull request Jun 23, 2023
@laurentschoelens laurentschoelens deleted the GH-2284 branch June 23, 2023 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants