[dna-issues] [JBoss JIRA] Updated: (DNA-457) Add JSR-170 Locking Optional Feature

[ https://jira.jboss.org/jira/browse/DNA-457?page=com.atlassian.jira.plugin... ] Brian Carothers updated DNA-457: -------------------------------- Attachment: DNA-457_for_review_2.patch Attached new patch (DNA-457_for_review_2.patch) In WorkspaceLockManager: 1) As the patch defines the code, the DnaLock.isLive() method is getting backdoor access to the map outside of a lock scope, and this seems like an error. FIXED - switched to use of ConcurrentHashMap and removed the lock 2) I'm not sure whether the read-write lock is guarding only the in-memory map or also the persisted content. If it's the latter, then what about other sessions created in other processes? If the read-write lock is to guard only the in-memory map, then the map probably can be changed to be a concurrent map and the read-write lock removed entirely, which could very well have a significant performance impact since this method is called once for each node in the path of a node that is being modified. Also, is the map just providing an in-memory cache of the content in the repository? FIXED - Switched DnaLock to ThreadSafe (since it has no mutable internal state but does rely on the thread-safe workspace locks map), moved workspaceLocks to be a ConcurrentHashMap and deleted the manual locking 3) The lock tokens appear to just be the string form of the locked node UUID. This is pretty easily forged, correct? Not sure that this is a bad thing, but it does give users a bit more rope to hang themselves. FIXED - The lock tokens are now UUIDs themselves that are independent of the underlying node's UUID. 4) The lockFor(UUID) method could be changed to accept an AbstractJcrNode, and thus would encapsulate a bit more of the WorkspaceLockManager internals. Of course, the lockFor(String token) would need to be changed a bit to reflect how the lockFor(UUID) worked. NOT FIXED - There's a place where I need to walk up the graph from the current node to the root to make sure that none of the parents have a deep lock. I can get the underlying o.j.d.g.Node easier than the AbstractJcrNode. If I changed to using AbstractJcrNode as the argument, I'd have to do more materialization (albeit with the same number of data retrieves). I'm still open to changing this, but it doesn't seem worth the materialization penalty to me off the cuff. The rest of my comments are _very_ minor nits. In AbstractJcrNode: 1) lock() will be called a lot (for every modification method on any node or property) ... seems like it'd be useful to cache some of the references. Also (minor nit) seems like the code could be cleaned up to reduce duplication by using a do-while loop. NOT FIXED - I don't think that you can do any non-trivial lock caching (i.e., cache the results of lockFor(nodeUuid)) since that could be changed outside of the current session. I did cache the workspace().session().lockManager() calls. I think I see what you're saying about refactoring into a do-while, but the lock handling is different for a lock on the current node vs. one of its parents if lockIsDeep is false. I think it's slightly more efficient as is, but I may be missing something. In JcrRepository: 1) JcrRepository.login(...) calls JcrRepository.getLockManager() and passes the WorkspaceLockManager reference into the JcrWorkspace constructor. This additional constructor parameter really isn't necessary, since the JcrWorkspace constructor could get it's own WorkspaceLockManager from the JcrRepository that is passed as a parameter to the constructor. FIXED - Good point. The redundant constructor arg is no more. 2) Some of the use of the concurrent lockManagers map is not really correct. Basically, one should never do multiple checks within a single method, since the map can change from one map method to the next. So, for example: WorkspaceLockManager getLockManager( String workspaceName ) { WorkspaceLockManager lockManager = lockManagers.get(workspaceName); if (lockManager != null) return lockManager; lockManagers.putIfAbsent(workspaceName, new WorkspaceLockManager(executionContext, this, workspaceName, locksPath)); return lockManagers.get(workspaceName); } should really be: WorkspaceLockManager getLockManager( String workspaceName ) { // Check for an existing lock manager ... WorkspaceLockManager lockManager = lockManagers.get(workspaceName); if (lockManager != null) return lockManager; // Need to create one if one hasn't just been created ... lockManager = new WorkspaceLockManager(executionContext, this, workspaceName, locksPath); WorkspaceLockManager newLockManager = lockManagers.putIfAbsent(workspaceName, lockManager); if ( newLockManager != null ) { // Some other thread just added this lock manager since our 'get' above ... lockManager = newLockManager; } return lockManager; } FIXED - Nice catch, thanks! -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira