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.

Sign up for GitHub

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

Jump to bottom

Make SessionRepositoryRequestWrapper::commitSession idempotent #1910

Closed
wants to merge 1 commit into from
Closed

Make SessionRepositoryRequestWrapper::commitSession idempotent #1910

wants to merge 1 commit into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Aug 31, 2021

Before this commit, commitSession could be called twice, which cause unnecessary access to sessionRepository

  1. closing response output stream by other filter
	SessionRepositoryFilter$SessionRepositoryRequestWrapper.commitSession() line: 217
	SessionRepositoryFilter$SessionRepositoryResponseWrapper.onResponseCommitted() line: 180
	SessionRepositoryFilter$SessionRepositoryResponseWrapper(OnCommittedResponseWrapper).doOnResponseCommitted() line: 227
	OnCommittedResponseWrapper$SaveContextServletOutputStream.close() line: 505
	LoggingBodyHttpServletResponse$ResponseServletOutputStream.close() line: 145 // other HttpServletResponse wrapper
	RestFilter.doFilter(ServletRequest, ServletResponse, FilterChain) line: 87 // other filter
	ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 189
	ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 162
  1. finally statement in SessionRepositoryFilter::doFilterInternal
	SessionRepositoryFilter$SessionRepositoryRequestWrapper.commitSession() line: 217
	SessionRepositoryFilter<S>.doFilterInternal(HttpServletRequest, HttpServletResponse, FilterChain) line: 145
	SessionRepositoryFilter<S>(OncePerRequestFilter).doFilter(ServletRequest, ServletResponse, FilterChain) line: 82
	ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 189
	ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 162

@quaff
Make SessionRepositoryRequestWrapper::commitSession idempotent
433116f 
Before this commit, commitSession could be called twice, which cause unnecessary access to sessionRepository
1. closing response output stream by other filter
	SessionRepositoryFilter$SessionRepositoryRequestWrapper.commitSession() line: 217
	SessionRepositoryFilter$SessionRepositoryResponseWrapper.onResponseCommitted() line: 180
	SessionRepositoryFilter$SessionRepositoryResponseWrapper(OnCommittedResponseWrapper).doOnResponseCommitted() line: 227
	OnCommittedResponseWrapper$SaveContextServletOutputStream.close() line: 505
	LoggingBodyHttpServletResponse$ResponseServletOutputStream.close() line: 145 // other HttpServletResponse wrapper
	RestFilter.doFilter(ServletRequest, ServletResponse, FilterChain) line: 87 // other filter
	ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 189
	ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 162
2. finally statement in SessionRepositoryFilter::doFilterInternal
	SessionRepositoryFilter$SessionRepositoryRequestWrapper.commitSession() line: 217
	SessionRepositoryFilter<S>.doFilterInternal(HttpServletRequest, HttpServletResponse, FilterChain) line: 145
	SessionRepositoryFilter<S>(OncePerRequestFilter).doFilter(ServletRequest, ServletResponse, FilterChain) line: 82
	ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 189
	ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 162
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 31, 2021
@quaff
Copy link
Contributor Author

quaff commented Sep 2, 2021

@vpavic Would you take a look at this?

@eleftherias
Copy link
Contributor

Hi @quaff, this is a duplicate of gh-1783.
For that reason I'm going to close this PR.

@eleftherias eleftherias closed this Sep 2, 2021
@eleftherias eleftherias self-assigned this Sep 2, 2021
@eleftherias eleftherias added status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 2, 2021
@quaff
Copy link
Contributor Author

quaff commented Sep 2, 2021

@eleftherias This one is more elegant than gh-1783.

@quaff quaff mentioned this pull request Sep 3, 2021
Closed
@quaff
Copy link
Contributor Author

quaff commented Feb 22, 2023

@vpavic Any thoughts?

@vpavic
Copy link
Contributor

vpavic commented Mar 8, 2023

@quaff this was considered for 3.0 however @rwinch and I decided to merge only #1909.

I really cannot recall the details at this moment, but there were certain scenarios where we thought that changes proposed by this PR weren't safe.

@quaff
Copy link
Contributor Author

quaff commented Mar 13, 2023

@quaff this was considered for 3.0 however @rwinch and I decided to merge only #1909.

I really cannot recall the details at this moment, but there were certain scenarios where we thought that changes proposed by this PR weren't safe.

Please leave a comment so we can avoid this.

@quaff
Copy link
Contributor Author

quaff commented Jul 20, 2023

@rwinch @vpavic Can we introduce an option let user decide whether to reduce commitSession?

quaff added a commit to quaff/spring-session that referenced this pull request Aug 10, 2023
@quaff
Introduce SessionRepositoryFilter::setCommitSessionOncePerRequest
eaeb9e4 
Fix spring-projects#1424
See spring-projects#1910
@quaff quaff mentioned this pull request Aug 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eleftherias eleftherias

Labels
status: duplicate A duplicate of another issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@quaff @eleftherias @vpavic @spring-projects-issues