SEC-1338: Poor concurrency in SessionRegistryImpl #1584

Closed
spring-issuemaster opened this Issue Dec 19, 2009 · 12 comments

1 participant

@spring-issuemaster

Nikita Koksharov (Migrated from SEC-1338) said:

In org.springframework.security.concurrent.SessionRegistryImpl for fields: principals, sessionIds used Collections.synchronizedMap's and for sessionsUsedByPrincipal field used Collections.synchronizedSet.

Because of it's poor concurrency of synchronized Collections we should used ConcurrentHashMap and CopyOnWriteArraySet instead.

@spring-issuemaster

Nikita Koksharov said:

Also we need to remove synchronized from "registerNewSession" method and use "putIfAbsent" on ConcurrentHashMap

@spring-issuemaster

Luke Taylor said:

Please submit a patch indicating the changes you think should be made.

Not something that will be implemented for 3 as it is essentially an optimization and the class has been working in its current form for a long time.

We will revisit it after the 3 release.

@spring-issuemaster

Luke Taylor said:

Again, please submit a patch indicating the suggested changes. Otherwise this will be closed as deferred/incomplete.

@spring-issuemaster

Nikita Koksharov said:

Patch attached

@spring-issuemaster

Luke Taylor said:

Thanks, but that doesn't look as if it is against the current head. Could you possibly produce a patch that just shows the relevant changes against the latest code.

@spring-issuemaster

Nikita Koksharov said:

Added SEC-1338_3.0.2.patch - new patch from 3.0.2 version

@spring-issuemaster

Luke Taylor said:

Thanks, I've applied the patch to the master branch.

For future reference, please try and submit patches as minimal change sets if possible (rather than an entire file replacement) as it makes them much easier to review.

Thanks again for your contribution.

@spring-issuemaster

Nikita Koksharov said:

SEC-1338_3.1.0.M2.patch should be applied, usage of CopyOfWriteArraySet replaced by ConcurrentHashMap due low speed of CopyOfWriteArraySet (it creates snapshot of whole collection each time when write occurred) and absence of ConcurrentHashSet implementation in Java 1.5. Only in Java 1.6 method Collections.newSetFromMap(...) exists which could be used to create true concurrent HashSet from ConcurrentHashMap, but it's only in Java 1.6.

@spring-issuemaster

Nikita Koksharov said:

SEC-1338_3.1.0.M2.patch should be applied to current master version in git.

@spring-issuemaster

Luke Taylor said:

Is this really necessary, given normal usage patterns? At the moment, it seems that the use of CopyOnWriteArraySet should not cause a problem. It is used to store the sessions for a principal which should remain small and will not be frequently written to.

@spring-issuemaster

Nikita Koksharov said:

I agree, we could keep CopyOnWriteArraySet in usage. You could back issue to "resolve" state :)

@spring-issuemaster

Luke Taylor said:

Ok. Thanks for your input :).

@spring-issuemaster spring-issuemaster added this to the 3.1.0.M2 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment