|
[
Permlink
| « Hide
]
Michael Neale added a comment - 11/Mar/07 05:51 AM
anyone for escalating this above a "minor" - its kind of a showstopper for some people ?
i agree with michael neale's judgement
(http://www.mail-archive.com/users@jackrabbit.apache.org/msg02503.html), changing priority to major. Any idea how this could be achieved?
If you use blocking locks, it is very hard to avoid deadlocks. Of course you can use an ordered sequence of locks for all requests to avoid deadlocks, but that would mean you need a list of all resources to be locked before you even start doing anything. After my experience with the Slide server I can only recommend non blocking locks. Such locks would make a request fail if it is not able to acquire all locks necessay instead of letting it wait. This would require a notion of a transaction, however. To drive this issue forward I created a patch that replaces the current rather hard coded use of a ReadWriteLock in SharedItemStateManager with an abstraction layer that allows alternative implementations.
The way locking currently works is implemented in DefaultISMLocking and an initial implementation of a more fine grained locking is available in FineGrainedISMLocking. FineGrainedISMLocking does not allow concurrent writes but at least allows reads while a long running write takes place and the read does not conflict with the write. I think this is the most common use case (save a big file but still allow other sessions to read, which is currently not possible). All tests pass with both the DefaultISMLocking and the FineGrainedISMLocking. The first patch only includes the structural changes moving away from the hard coded ReadWriteLock in the SharedItemStateManager to the ISMLocking interface. It does not change the locking semantics that are currently in place. The second patch is the proposal for a more fine grained locking strategy. Comments are welcome. As far as I can tell the FineGrainedISMLocking implementation is prone to deadlocks. At least when a DB is used as storage. Such a deadlock can occur distributed between DB and the Jackrabbit locking. The DB can not resolve the deadlock, as internally it looks like there is none. A transaction timeout in the DB is the only way to resolve such a situation.
In any case there should be a timeout for Jackrabbit locks. For one to to free locks that have never been freed, because some thread forgot to free them. Additionally, to make sure deadlocks between Jackrabbit and DB can be resolved quickly. Oliver, thank you for your comment. However I do not see how a deadlock may occur between jackrabbit and the DB used in a persistence manager. Deadlocks always require that locks are acquired in a circular way. That's never the case between Jackrabbit and the DB. A call that returns from the DB will always have released all DB locks.
I'd appreciate if you were a bit more specific, e.g. provide a wait-for graph that shows a deadlock situation. > In any case there should be a timeout for Jackrabbit locks. For one to to free locks that have > never been freed, because some thread forgot to free them. I disagree. This would clearly be a bug in Jackrabbit and should rather be fixed than worked around with timeouts. I briefly reviewed the patches, good work! I like how you've applied the strategy pattern.
My main concern is that this still doesn't address the concurrency limitations in the current persistence managers. Should that be handled as a separate issue? A more general issue is that this whole ISM core keeps getting more complex, and adding 1+2 new interfaces and all the implementing classes doesn't really help things. I'm not sure what to do about that... Also, it would be nice if we had at least a few parallel test cases for excercising such code. Jukka, thanks for your comments.
> My main concern is that this still doesn't address the concurrency limitations in the current > persistence managers. You are right, the proposed changes will not solve the concurrency limitations completely, but at least they should improve the current situation where no reads are possible through the SISM when a write is taking place. I think truely fine grained locking will not be possible with the current design and thus would require major re-structuring in the jackrabbit core. Something I'd rather like to postpone for a 2.0. > [...] limitations in the current persistence managers. Should that be handled as a separate issue? There are a number of other issues that are related. Even if persistence managers are able to perform concurrent writes, the index will have to support that too otherwise concurrent writes in the PM will be of limited use. And then there's the question how to process (synchronous) events for concurrent writes? > A more general issue is that this whole ISM core keeps getting more complex, and adding 1+2 > new interfaces and all the implementing classes doesn't really help things. I actually didn't want to make it more complex but rather easier to understand. I think the interfaces are of some value because they describe the locking contract independant of the implementation. They also allow to easily try out a different locking implementation / strategy. > I'm not sure what to do about that... I'm also not 100% happy with the proposed changes, but at least it solves the most annoying problem with the SISM, that it locks up completely during a large write. I see the following options: - We don't change anything right now and do it better the next time (NGP, jackrabbit 2.0, ...) - Redesign the core now to better handle concurrency - Accept the patch ISMLocking.patch and discuss a locking implementation/strategy that's better suited than the current one. I'm obviously in favour of the last option ;) because it gives us the most bang for the buck and is by far less risky than option two. > Also, it would be nice if we had at least a few parallel test cases for excercising such code. I agree. I already created some test cases that perform concurrent versioning operations and basic content modifications. So if anyone is interested, feel free to write more of them. The class AbstractConcurrencyTest and its sub classes are a good starting point. > I'm obviously in favour of the last option ;) because it gives us the most bang for the buck and is by far less risky than option two.
Agreed. Note that without solving the concurrency issues in persistence managers (both db and bundle) we're still stuck with fully serialized backend access. But this is one step in the correct direction; the second step would be to start removing the synchronization on the persistence managers. > - Accept the patch ISMLocking.patch and discuss a locking implementation/strategy that's better suited than the current one.
+1 btw, i did a couple of test runs using ConcurrentReadWriteTest against a) current trunk b) a) with ISMLocking.patch applied (using DefaultISMLocking) c) b) with FineGrainedISMLocking.patch applied (using FineGrainedISMLocking) environment: macbook pro (intel, 2ghz), os-x 10.4.9, jre 1.4.2 results: a) ** ~2'300 #writes ~2'300'000 #reads b) ** ~2'300 #writes ~2'300'000 #reads c) ** ~1'700 #writes ~2'300'000 #reads => c) shows a significant decrease in #writes while #reads doesn't seem to be affected. i guess that FineGrainedISMLocking still has room for improvement ;) Here's a new version of FineGrainedISMLocking.
The performance should be better compared to the last version. I'm not sure the test case is very representative though. Lot of monitor contention happens between the write thread and readers when item states are notified and the cache maps need to be accessed. I think finer grained locking makes this situation even worse because it allows more concurrency on the caches. Applied the patches ISMLocking.patch and FineGrainedISMLocking-v2.patch with one slight change: the SISM used the default implementation for now.
Committed in revision: 540944 There are some remaining issues before we can use FineGrainedISMLocking: - using FineGrainedISMLocking allows for concurrent access to item state caches, which were not possible before. We must ensure that all item state caches are prepared for that. - do extensive testing - we should check again if the performance is ok The chart shows the writes per second of the writer thread using the
ConcurrentReadWriteTest for DefaultISMLocking and FineGrainedISMLocking. The duration of the test is 120 seconds. It seems that saving a single large binary property still (revision 549230) completely blocks read access to other nodes. See my last comment on
Forget my last comment, the svn trunk still has DefaultISMLocking as the default.
It seems that during the time a versioning operation is in progress (writer) it is not possible to establish a new session to the repository. This seems to be so because access to the root node of the version histories (/jcr:system/jcr:versionStorage) is required to create a session and this node is included on the active writer changelog. In other words the reader need to access an item (root node of the version storage) that has a dependency on the active writer changelog.
From XAVersionManager constructor try { state = (NodeState) stateMgr.getItemState(vMgr.getHistoryRootId()); } catch (ItemStateException e) { ... } (An XAVersionManager is instantiated during the instantiation of an XASessionImpl) Note: I've observed this in an XA environment. Am I wrong or right ? What may be the consequences of allowing a reader to get a read lock in this situation even if the writer's changelog includes the VERSION_STORAGE_NODE_ID ? > What may be the consequences of allowing a reader to get a read lock in this situation even if the writer's changelog includes
> the VERSION_STORAGE_NODE_ID ? That's probably a bad idea. It may happen that a reader sees inconsistent data. But anyway, I think a versioning operation will modify only the version storage root node in rare cases. Merely when the version storage is nearly empty. As soon as there are enough versions created the version storage root node has all possible child nodes (named 00, 01, ... ff) and is not modified anymore. From http://www.mail-archive.com/users@jackrabbit.apache.org/msg02503.html
> for people wanting to store large blobs (something that people would look at using JCR for) - this is a showstopper. If reading/writing large objects blocks others, it is showstopper. But Data Store should solve this: http://issues.apache.org/jira/browse/JCR-926 Is it still a problem? Are there other concurrency problems? This patch adds a lockingStrategy attribute to the Workspace and Versioning element tags. It's possible values are default, fine, and fine+ (this one was a POC we did here).
Esteban, thank you for your patch. I've modified it slightly and used a separate configuration element with a class attribute. I didn't include your changes to the FineGrainedISMLocking because I think they may lead to inconsistencies. As described easier in this issue, once the version storage in sufficiently populated, the login will not block anymore when a version operation is in progress.
I have also added a new version of the DTD (1.4) and updated all repository.xml files I found in the sources. Once the changes are committed I will add the DTD to the website. Please note that this change is backward compatible. The ISMLocking element is optional and defaults to DefaultISMLocking (the currently hard coded implementation). Committed ConfigurableISMLocking.patch in revision: 605173
Please note that there are still some open synchronization issues when using FineGrainedISMLocking! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
Fine grained locking in SharedItemStateManager
