Skip to content

SessionRegistryImpl should use computeIfAbsent #5834

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
jzheaux opened this issue Sep 11, 2018 · 3 comments
Closed

SessionRegistryImpl should use computeIfAbsent #5834

jzheaux opened this issue Sep 11, 2018 · 3 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: first-timers-only An issue that can only be worked on by brand new contributors type: enhancement A general enhancement
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Sep 11, 2018

Java 8 introduced computeIfAbsent on ConcurrentHashMap:

If the specified key is not already associated with a value,
attempts to compute its value using the given mapping function
and enters it into this map unless {@code null}. The entire
method invocation is performed atomically, so the function is
applied at most once per key.

Since SessionRegistryImpl uses ConcurrentHashMap and since it uses it as <String, List<?>>, it seems like an ideal circumstance to make the update to principals atomic.

Instead of:

Set<String> sessionsUsedByPrincipal = principals.get(principal);

if (sessionsUsedByPrincipal == null) {
	sessionsUsedByPrincipal = new CopyOnWriteArraySet<>();
	Set<String> prevSessionsUsedByPrincipal = principals.putIfAbsent(principal,
			sessionsUsedByPrincipal);
	if (prevSessionsUsedByPrincipal != null) {
		sessionsUsedByPrincipal = prevSessionsUsedByPrincipal;
	}
}

sessionsUsedByPrincipal.add(sessionId);

It could do:

principals.computeIfAbsent(principal, key -> new CopyOnWriteArraySet<>())
	.add(sessionId);
@jzheaux jzheaux added the type: enhancement A general enhancement label Sep 11, 2018
@jzheaux jzheaux added this to the General Backlog milestone Dec 3, 2018
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) status: first-timers-only An issue that can only be worked on by brand new contributors labels Dec 3, 2018
@rmartinus
Copy link
Contributor

I can work on this as my first contribution to spring security

@rwinch
Copy link
Member

rwinch commented Dec 3, 2018

@rmartinus Thanks the issue is yours! If you need any help, please let us know.

@rmartinus
Copy link
Contributor

@jzheaux @rwinch please review my PR when you get a chance

rmartinus added a commit to rmartinus/spring-security that referenced this issue Dec 4, 2018
@rwinch rwinch modified the milestones: General Backlog, 5.2.0.M1 Dec 5, 2018
rwinch pushed a commit that referenced this issue Dec 5, 2018
@rwinch rwinch self-assigned this Dec 5, 2018
rwinch pushed a commit that referenced this issue Dec 5, 2018
rwinch pushed a commit that referenced this issue Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: first-timers-only An issue that can only be worked on by brand new contributors type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants