I tried implementing the suggested code, and while it works in eclipse, when running the service, the first run passes, but all subsequent requests hang while getting object from the cache.
After looking more closely at the caching code, it is already using a blocking cache, which by default will return null on the first get and lock indefinitely on subsequent gets until the cache is populated. Perhaps it's the combination of our locks and ehcache's locks are causing issues??? I do think the blocking cache makes our lock code unnecessary as the cache is already handling multiple threads. Thoughts? I would like to add ability to configure the maxtimeout on the blocking cache so indefinite blocks can't happen. If the timeout triggers on multiple gets to the blocking cache, it is possible that load graph could be called simultaneously. Perhaps this method needs to be synchronized for this hopefully unlikely scenario. ----- Original Message ----- From: "Emmanuel Lécharny" <[email protected]> To: [email protected] Sent: Tuesday, March 8, 2016 1:19:06 PM Subject: Re: LDAP Connection Management Le 08/03/16 17:22, Shawn McKinney a écrit : >> On Mar 8, 2016, at 8:40 AM, Christopher Harm <[email protected]> wrote: >> >> The Javadocs state that upgrading from a readLock to a writeLock is not >> possible, and I've been reading other material that states that this is >> because that operation is deadlock prone. The function is pretty simple, in >> that it doesn't do anything inside the read lock after it's acquired the >> write lock. I suggest that we de-couple the read and write lock scopes as >> such.... >> >> private static SimpleDirectedGraph<String, Relationship> getGraph(String >> contextId) { >> ReadWriteLock hierLock = HierUtil.getLock(contextId, HierUtil.Type.ROLE); >> String key = getKey(contextId); >> >> SimpleDirectedGraph<String, Relationship> graph = null; >> hierLock.readLock().lock(); >> try { >> graph = (SimpleDirectedGraph<String, Relationship>) roleCache.get(key); >> if (graph != null) { >> return graph; >> } >> } finally { >> hierLock.readLock().unlock(); >> } >> >> hierLock.writeLock().lock(); >> try { >> graph = (SimpleDirectedGraph<String, Relationship>) roleCache.get(key); >> if (graph != null) { >> return graph; >> } >> >> graph = loadGraph(contextId); >> >> return graph; >> } finally { >> hierLock.writeLock().unlock(); >> } >> } > +1 looks good. (Much simpler) I think Shawn is right when he said that we first should check if the protection against concurrent access is really needed. That's a bit foggy for me (it was 2 years ago), and I'm afraid that I suggested to protect this code just for the sake of it. I'd liek to get a second review of this specific piece of code which *may* not need synchronization at all...
