Don't use double checked locking for CertLdapLoginModule initialization #276
Conversation
| if (options.containsKey(ROLES_CACHE_EXPIRY_MS)) { | ||
| rolesCacheExpiry = Integer.parseInt((String)options.get(ROLES_CACHE_EXPIRY_MS)); | ||
| } | ||
| synchronized(LdapRolesProvider.class) { |
There was a problem hiding this comment.
This will synchronize every time initialize is called. Why not use volatile?
There was a problem hiding this comment.
Volatile doesn't mean what you think, either
A commonly suggested nonfix is to declare the resource field of SomeClass as volatile. However, while the JMM prevents writes to volatile variables from being reordered with respect to one another and ensures that they are flushed to main memory immediately, it still permits reads and writes of volatile variables to be reordered with respect to nonvolatile reads and writes. That means -- unless all Resource fields are volatile as well -- thread B can still perceive the constructor's effect as happening after resource is set to reference the newly created Resource.
If I understand this correctly (and unless something has changed, it's a really old article), volatile will not affect the properties of LdapRolesProvider, unless they are volatile as well.
There is also https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom, but it only works for initialization which cannot fail, so not good.
|
afaik, volatile is a memory barrier. A write to a volatile variable will
force all waiting writes to be flushed, so it should be safe to use
volatile.
…On Wed, Dec 14, 2016 at 3:28 AM, Marek ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In auth/src/main/java/com/redhat/lightblue/rest/auth/jboss/
CertLdapLoginModule.java
<#276>:
> - }
- if (options.containsKey(DEBUG)) {
- ldapConf.debug(Boolean.parseBoolean((String)options.get(DEBUG)));
- }
- if (options.containsKey(KEEP_ALIVE)) {
- ldapConf.keepAlive(Boolean.parseBoolean((String)options.get(KEEP_ALIVE)));
- }
- if (options.containsKey(POOL_MAX_CONNECTION_AGE_MS)) {
- ldapConf.poolMaxConnectionAgeMS(Integer.parseInt((String)options.get(POOL_MAX_CONNECTION_AGE_MS)));
- }
-
- int rolesCacheExpiry = 5*60*1000; // default 5 minutes
- if (options.containsKey(ROLES_CACHE_EXPIRY_MS)) {
- rolesCacheExpiry = Integer.parseInt((String)options.get(ROLES_CACHE_EXPIRY_MS));
- }
+ synchronized(LdapRolesProvider.class) {
Volatile doesn't mean what you think, either
A commonly suggested nonfix is to declare the resource field of SomeClass
as volatile. However, while the JMM prevents writes to volatile variables
from being reordered with respect to one another and ensures that they are
flushed to main memory immediately, it still permits reads and writes of
volatile variables to be reordered with respect to nonvolatile reads and
writes. That means -- unless all Resource fields are volatile as well --
thread B can still perceive the constructor's effect as happening after
resource is set to reference the newly created Resource.
(from http://www.javaworld.com/article/2074979/java-
concurrency/double-checked-locking--clever--but-broken.html)
If I understand this correctly (and unless something has changed, it's a
really old article), volatile will not affect the properties of
LdapRolesProvider, unless they are volatile as well.
There is also https://en.wikipedia.org/wiki/Initialization-on-demand_
holder_idiom, but it only works for initialization which cannot fail, so
not good.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#276>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADgDDc6C3K3N_yEOa2qDDIJ56tZxY8YFks5rH8TfgaJpZM4LL6gS>
.
|
|
That article from 2001 probably speaks of volatile implementation prior to jdk5. So I'm following your recommendation. |
https://en.wikipedia.org/wiki/Double-checked_locking
Locking on each getRoleSets call (every Lightblue request). Hope it does not affect performance.
LdapConfiguration will be initialized only once - this will improve performance.