-
Notifications
You must be signed in to change notification settings - Fork 38.4k
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
DefaultSimpUserRegistry Use ReentrantLock to Prevent Pinning #34538
Conversation
Signed-off-by: SeungHun Choi <csh0034@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicely done
|
||
this.sessionLock.lock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.sessionLock.lock(); | |
sessionLock.lock(); |
There is no overloaded field that makes the this.
access required, right? Both are correct, but if there is no ambiguity, it's more concise to leave it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your valuable feedback.
I actually considered this when making the code changes.
Looking at other classes that use ReentrantLock, I noticed that they all include this.
so I decided to keep it for consistency.
(e.g., SubProtocolWebSocketHandler
, InMemoryWebSessionStore.ExpiredSessionChecker
)
Are you experiencing any actual pinning there? The code within those |
@jhoeller |
Alright, let's keep it as-is then since we keep using such synchronization around local data structure operations in other places as well. Thanks for your efforts, in any case! |
modified the part where pinning occurs in DefaultSimpUserRegistry due to the use of synchronized when using VT
I would appreciate any feedback 😃