8000 KEYCLOAK-11297 Decreases possibility of deadlock for offline tokens by k-tamura · Pull Request #6356 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

KEYCLOAK-11297 Decreases possibility of deadlock for offline tokens - #6356

Closed
k-tamura wants to merge 1 commit into
keycloak:masterfrom
k-tamura:KEYCLOAK-11297
Closed

KEYCLOAK-11297 Decreases possibility of deadlock for offline tokens#6356
k-tamura wants to merge 1 commit into
keycloak:masterfrom
k-tamura:KEYCLOAK-11297

Conversation

@k-tamura
@k-tamura k-tamura commented Oct 4, 2019
Copy link
Copy Markdown
Contributor

Please refer to KEYCLOAK-11297.

hmlnarik
hmlnarik suggested changes Nov 5, 2019
View reviewed changes
@hmlnarik hmlnarik left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the analysis in JIRA and the PR. I find introduction of findExpiredUserSessions and the call of it in JpaUserSessionPersisterProvider.java redundant though. DELETE statement would use the index just the same as the newly introduced SELECT, and would perform the job in a single shot instead of two separate statements, hence the benefit of changing transaction isolation level also diminishes.

As for the index, this is the crucial improvement that should get in, but only after https://issues.jboss.org/browse/KEYCLOAK-11908 is resolved

@k-tamura
k-tamura commented Nov 7, 2019
Copy link
Copy Markdown
Contributor Author

Thank you for reviewing.

DELETE statement would use the index just the same as the newly introduced SELECT, and would perform the job in a single shot instead of two separate statements,

The DELETE statement can cause deadlock even if target records don't exist. I added the SELECT query (findExpiredUserSessions) because deadlock never (0%) occurs if the DELETE statement is not issued. In addtion, I added query.setMaxResults(1); (it means limit 1 in SQL) to the SELECT statement since RDBMS quickly returns the result of the SELECT statement (=whether target records exist or not) just after RDBMS found a target record. So the SELECT statement positively affects the performance of the scheduled task in total.

hence the benefit of changing transaction isolation level also diminishes.

I have confirmed that the possibility of deadlock is decreased by changing transaction isolation level. There are no disadvantages of the changes because the priority of the scheduled task is lower than online threads. Even if the task fails to delete all expired offline records, there are no problems because the task is repeatedly executed.

As for the index, this is the crucial improvement that should get in, but only after https://issues.jboss.org/browse/KEYCLOAK-11908 is resolved

I understand. This PR suspends until KEYCLOAK-11908 is resolved.

@mposolda mposolda added the Hold label Jan 3, 2020
@mposolda
mposolda commented Jan 3, 2020
Copy link
Copy Markdown
Contributor

Postpone this to "Hold" as it seems that we won't have time to look at KEYCLOAK-11908 before Keycloak 9.0. @hmlnarik are you ok with that?

@stianst stianst added this to the 10.0.0 milestone Mar 17, 2020
@stianst stianst removed the Hold label Apr 20, 2020
@stianst stianst modified the milestones: 10.0.0, 11.0.0 Apr 21, 2020
@mposolda mposolda modified the milestones: 11.0.0, Backlog May 4, 2020
@mposolda mposolda added the Hold label May 4, 2020
@mposolda
mposolda commented May 4, 2020
Copy link
Copy Markdown
Contributor

@hmlnarik Feel free to correct me, but it looks that https://issues.redhat.com/browse/KEYCLOAK-11908 won't happen anytime soon? Hence I am adding this PR to the "Hold" state.

If we have contribution for the KEYCLOAK-11908, it will likely help with this PR as well. But I think the plan for this might need to be discussed on the ML first.

@stianst stianst removed this from the Backlog milestone May 8, 2020
@mposolda
mposolda commented Jun 9, 2020
Copy link
Copy Markdown
Contributor

@k-tamura Thanks for the contribution. I am closing this PR due the fact that the PR probably won't happen anytime soon as the KEYCLOAK-11908 won't happen anytime soon as well. And we want to reduce the amount of pull requests, which is very big :) Feel free to re-open once we have KEYCLOAK-11908 or feel free to start discussion on keycloak-dev if you think that something should be improved in different manner. Sorry for late notice and Thanks for understanding.

@mposolda mposolda closed this Jun 9, 2020
@k-tamura
k-tamura commented Jan 13, 2021
Copy link
Copy Markdown
Contributor Author

Hi @hmlnarik , @stianst , @mposolda

I would like to re-open this ticket because KEYCLOAK-11908 has been merged. Could you review again?

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

Reviewers

1 more reviewer
@hmlnarik hmlnarik hmlnarik requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mposolda mposolda

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@k-tamura @mposolda @hmlnarik @stianst
0