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

[Randall Hauch (JIRA)]

    [ https://jira.jboss.org/jira/browse/DNA-457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12489683#action_12489683 ] 

Randall Hauch commented on DNA-457:
-----------------------------------

For the most part, this is a great first step!  I do have a few questions, though.

The general approach of persisting the lock information in /jcr:system/dna:locks seems like a good approach - it provides master persisted lock information accessible to all distributed processes.  The organization seems pretty good - /jcr:system/dna:locks/<lock_for_node_given_node_uuid>, where the lock node contains the information about the node being locked as well as the information about the owner of the lock.  And the restriction to mix:referenceable nodes is obvious, since the nodes' UUIDs are used in the name of the DNA lock.

Considering that this approach doesn't work with nodes that are not mix:referenceable, would it work to use the UUID for the lock (rather than the UUID of the locked node) as the node name?  The UUID and/or path could then be kept in the lock info node (and because this node is locked, only the session with the lock token can modify the node and possibly move the node, and in that case the session could update the path in the lock info).  This would make it more difficult to quickly look up a lock given a node, but isn't that the point of the in-memory map?  The in-memory map could be changed to be keyed by Location, which would work by path and/or UUID and/or by other ID properties.  I'm not sure whether this would even work, because I might be missing some aspects of the current design or failing to recognize all of the issues with this alternative.  Thoughts?

Back to the current patch.  Here are a few comments:

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.
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?
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.
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.

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.

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.
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;
    }

> Add JSR-170 Locking Optional Feature
> ------------------------------------
>
>                 Key: DNA-457
>                 URL: https://jira.jboss.org/jira/browse/DNA-457
>             Project: DNA
>          Issue Type: Feature Request
>          Components: JCR
>    Affects Versions: 0.5
>            Reporter: Brian Carothers
>             Fix For: 1.0
>
>         Attachments: DNA-457_for_review.patch, DNA-457_WIP.patch
>
>
> JSR-170 defines several optional features.  The locking feature allows users to lock nodes (and, optionally, their descendants) until the session terminates or until they are programatically unlocked.

-- 
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