search

lightblue-platform / lightblue-rest

Data access layer as service REST API
GNU General Public License v3.0
9 stars 16 forks source link

Don't use double checked locking for CertLdapLoginModule initialization #276

Closed paterczm closed 7 years ago

paterczm commented 7 years ago

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.

bserdar commented 7 years ago

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 notifications@github.com wrote:

@paterczm commented on this pull request.

In auth/src/main/java/com/redhat/lightblue/rest/auth/jboss/ CertLdapLoginModule.java https://github.com/lightblue-platform/lightblue-rest/pull/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 = 5601000; // 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 https://github.com/lightblue-platform/lightblue-rest/pull/276, or mute the thread https://github.com/notifications/unsubscribe-auth/ADgDDc6C3K3N_yEOa2qDDIJ56tZxY8YFks5rH8TfgaJpZM4LL6gS .

paterczm commented 7 years ago

That article from 2001 probably speaks of volatile implementation prior to jdk5. So I'm following your recommendation.