[ModeShape 5.x] Automated cleanup of shallow open-scoped locks

[illia.khokholkov]

In our application we utilize shallow open-scoped locks. The typical usage pattern is provided below:

// lock
try {
    // do something
    // persist changes
} finally {
    // unlock (if session has pending changes, i.e. the call to persist them failed, discard them first)
}

It is possible that the JVM dies after the lock is obtained for a given node so that we can never reach the code to unlock the node. As far as I know, there is a setting in the ModeShape repository configuration file that allows to configure an interval at which garbage collection is performed. As a part of that process, the expired locks will be removed. The default value is 24 hours and there is a note that says that cleanup is a very resource intensive operation and therefore has to be done when the system experiences the least amount of load. Here are some questions related to lock cleanup in case of a fatal JVM error or host going down:

1. If I am reading the source code correctly, the lock expiration time is set to 24 hours minus 10 minutes. This seems like a lot of time for the node to remain locked, especially if this is a root node of an application and a new node has to be added as a direct child. Is there a way I can configure the lock expiration separately from the value I use for the overall garbage collection process? If not, based on your experience, is it fine to perform more frequent global cleanup and what kind of performance implications am I going to face?
2. Is there a way to remove a lock by manually going to the database (assuming it is persisted there in one way or another and it is not JGroups that maintains it) or will such an action mess up all members of the cluster and possibly permanently ruin the integrity of the entire repository?

Many thanks in advance, any help is appreciated.

[hchiorean]

1. If I am reading the source code correctly, the lock expiration time is set to 24 hours minus 10 minutes. This seems like a lot of time for the node to remain locked, especially if this is a root node of an application and a new node has to be added as a direct child. Is there a way I can configure the lock expiration separately from the value I use for the overall garbage collection process? If not, based on your experience, is it fine to perform more frequent global cleanup and what kind of performance implications am I going to face?

when you lock a node you can pass in a timeout value which can be used to control the timeout for that particular lock. Any positive value different from Long.MAX_VALUE should work (see modeshape/RepositoryLockManager.java at master · ModeShape/modeshape · GitHub ). You can also try setting a lower global GC value and see if how works. There are no silver bullet rules for these settings, so you should experiment with whatever works best for your use case.

   2. Is there a way to remove a lock by manually going to the database (assuming it is persisted there in one way or another and it is not JGroups that maintains it) or will such an action mess up all members of the cluster and possibly permanently ruin the      integrity of the entire repository?

did you try using the LockManger#unlock method ? That should normally remove the lock for a given node. 

Ideally for these types of scenarios (unexpected JVM crash), beside the configuration options, your application should keep track of what it managed to lock successfully or not and if there are any "dangling" locks on restart, it should explicitly unlock them via the JCR Lock API.

[illia.khokholkov]

when you lock a node you can pass in a timeout value which can be used to control the timeout for that particular lock

I am aware of that feature and it is being utilized in the code I am working on. I was mostly referring to the global timeout setting applicable to all locked nodes, which would be configurable in some way, e.g. repository configuration file. I was mistakenly expecting more options to be available to gain a fine-grained control over processes that GC involves.

did you try using the LockManger#unlock method ? That should normally remove the lock for a given node.

Yes, this is what is used to unlock the node and it functions great as long as we can get up to that point, i.e. no fatal JVM errors happen.

your application should keep track of what it managed to lock successfully or not and if there are any "dangling" locks on restart, it should explicitly unlock them via the JCR Lock API

This is a great suggestion, unfortunately, I thought this is what repository would do for me and it does so (to some extent, with GC process). Any way, thank you very much for your feedback.

[illia.khokholkov]

I was mostly referring to the global timeout setting applicable to all locked nodes, which would be configurable in some way, e.g. repository configuration file. I was mistakenly expecting more options to be available to gain a fine-grained control over processes that GC involves.

hchiorean, if I come up with a pull request to expose configuration options specific to lock cleanup, e.g. when to start and how frequently the cleanup should be made, would you even consider it?

[hchiorean]

illia.khokholkov I'm happy to look at any PR which enhances/fixes issues, but we already have a means of configuring the GC process: modeshape/repo-config-garbage-collection.json at master · ModeShape/modeshape · GitHub

Not sure what other configuration options could/should be exposed

[illia.khokholkov]

hchiorean, I have created JIRA [1] and corresponding PR [2]. Please, take a look at them whenever you get a chance. I hope that use case I am interested in is clearly explained. Thank you.

[1] [MODE-2631] Allow for better granularity when configuring JCR lock cleanup process - JBoss Issue Tracker

[2] MODE-2631: allow for better granularity when configuring JCR lock cleanup process. by dnillia · Pull Request #1596 · Mod…

[rhauch]

First of all, there are two kinds of locks within JCR:

1. Session-scoped locks are owned by the session that created them, and remain locked until the lock expires, the session is used to explicitly unlocked it, or the session is closed.
2. Open-scoped locks are initially owned by (or attached to) the session that created them, and remain locked until the lock expires or is unlocked by any session to which the lock's token is attached.

Session-scoped locks are much easier to use, but don't fit all usecases. Open-scoped locks are more flexible, but require the application using ModeShape to ensure that the lock tokens are preserved separately so that it can later be attached to another session since, presumably, an open-scoped lock is being used to avoid co-expiration with the initial session. (ModeShape provides the LockManager.addLockToken, LockManager.removeLockToken and LockManager.getLockTokens methods to attach tokens to and detach tokens from sessions.) As you point out above, the application needs to do this in a way that survives failures.

Expiration times are one mechanism by which you can help increase the fault tolerance of the system so that either session-scope or open-scoped locks are properly cleaned up. If you don't like the default timeout (which is quite long, BTW), simply use an explicit timeout. We'd probably even accept a PR that changes the default to something more sensible, even if that is more arbitrary.

Finally, garbage collection (at least the portion related to locks) will clean up locks that have expired, but it's not the only place this is done. When you attempt to obtain a lock on a node, if another lock on the node exists but has already expired, then the lock manager will clean up the expired lock and create a new lock for you. If a lock on the node exists but is not expired, then the lock manager rejects your request to obtain a lock. So, while it may seem useful to clean up locks more frequently, it shouldn't really be required if you are using sensible lock expiration times and are properly and durably handling any lock tokens for open-scoped locks.

I hope this helps.

[illia.khokholkov]

rhauch, thank you for joining this thread and providing your input.

When you attempt to obtain a lock on a node, if another lock on the node exists but has already expired, then the lock manager will clean up the expired lock and create a new lock for you. If a lock on the node exists but is not expired, then the lock manager rejects your request to obtain a lock.

This is the ideal scenario for me. In fact, I would not even care about automated lock cleanup process that is performed by the background thread, if lock manager can unlock the node (the lock for which has expired) the moment I attempt to lock it. Unfortunately, I must be reading the source code incorrectly, because it does not seem to be that such a kind of behavior would be allowed. Please, consider the source code provided below.

The "JcrLockManager.lock(AbstractJcrNode, boolean, boolean, long, String)", as provided in [1], will not delegate a call to the "RepositoryLockManager.lock(JcrSession, CachedNode, boolean, boolean, long, String)" if the node is already locked:

...
if (node.isLocked()) {
    throw new LockException(JcrI18n.alreadyLocked.text(node.location()));            
}
...
// Try to obtain the lock ...
ModeShapeLock lock = lockManager.lock(session, node.node(), isDeep, isSessionScoped, timeoutHint, ownerInfo);

The "RepositoryLockManager.lock(JcrSession, CachedNode, boolean, boolean, long, String)", on the other side, is indeed capable of unlocking the node with an expired lock, as provided in [2]:

...
if (expirationDate.isBefore(now)) {
    //remove the lock from the system area
    systemContent.removeLock(lock);
    ...
}

Please, let me know if my understanding of the existing source code is incorrect.

[1] modeshape/JcrLockManager.java at modeshape-5.1.0.Final · ModeShape/modeshape · GitHub

[2] modeshape/RepositoryLockManager.java at modeshape-5.1.0.Final · ModeShape/modeshape · GitHub

[rhauch]

It depends on what "node.isLocked()" actually does. For example, I would expect it to return "false" if the lock has already expired. If not, then we do probably need to adjust the logic, ensuring that whatever changes still adhere to the JCR compatibility/compliance tests.

[illia.khokholkov]

It depends on what "node.isLocked()" actually does.

Fair point, because my understanding was purely based on the JavaDoc from the JCR API:

Returns <code>true</code> if this node is locked either as a result of a lock held by this node or by a deep lock on a node above this node; otherwise returns <code>false</code>. This includes the case where a repository does not support locking (in which case all nodes are "unlocked" by default).

I took a look at the implementation, and it boils down to the method [1], which does not appear to be dealing with lock expiration.

If not, then we do probably need to adjust the logic, ensuring that whatever changes still adhere to the JCR compatibility/compliance tests.

It sounds like you would not be opposed to the improvement logged against ModeShape, so I am going to create a new "Feature Request", assuming I can confirm through the test case that unlock is indeed not happening. Thank you.

[1] modeshape/JcrLockManager.java at modeshape-5.1.0.Final · ModeShape/modeshape · GitHub

[rhauch]

Illia Khokholkov wrote:

...

It sounds like you would not be opposed to the improvement logged against ModeShape, so I am going to create a new "Feature Request", assuming I can confirm through the test case that unlock is indeed not happening. Thank you.

Sure, go ahead and log an issue about correcting/improving the logic, but please log that as a "Bug" since is changing the behavior. If you do that, do you agree that the exiting JIRA and pull request are no longer necessary? 

[illia.khokholkov]

Sure, go ahead and log an issue about correcting/improving the logic, but please log that as a "Bug" since is changing the behavior.

The bug [1] has been logged.

If you do that, do you agree that the exiting JIRA and pull request are no longer necessary?

Yes, absolutely. However, I am not familiar with the source code enough to submit a PR for the bug I just logged, assuming it is indeed a valid problem. If, by any chance, it can be included in ModeShape 5.2, that would be amazing. Although, if it gets scheduled for some future release, I would still like the PR for additional lock cleanup configuration options to be considered. Thank you.

[1] [MODE-2633] Unable to lock the node for which existing lock expired - JBoss Issue Tracker

[rhauch]

Thanks for logging that issue. So, assuming that this gets fixed/correct, then regarding your original issue and pull request I agree with Horia that we don't want to complicate the configuration and garbage collection process, especially when that same garbage collection activity really is just cleaning up already-expired locks that should not prevent obtaining new locks.