ModeShape/modeshape — pull request #1108 by rhauch (2014-05-06 21:15:33+00:00)
Title: MODE-2202 Refactored multi-session transactional tests to use separate threads

The `JcrRepositoryTest` class contains two test methods that verified the behavior of two sessions when one of them is using a transaction, specifically that the changes made by a session using a transaction become visible (via events, navigation or query) to another session only when the transaction commits and that the changes never become visible if the transaction is rolled back.

The previous version of the tests used a single thread by interleaving actions for each of the separate sessions and checks. Unfortunately, this is unrealistic, since technically both sessions might be able to "see" the transaction if that transaction is associated with the thread.

MODE-2202 was logged by @hchiorean because one of these tests (in particular, the rollback test) was failing because it would leak the changes made by the session using a transaction so that they are incorrectly visible to the other session. I was not able to reproduce this failure locally.

By debugging the code, I did verify, at least on my machine, that no events were being prematurely fired from the session before the transaction was committed, and that the workspace caches were *not* incorrectly receiving events and purging entries from their state. Besides, the workspace caches would only contain the truly persisted-and-committed changes; it is the `TransactionalWorkspaceCache` instance used by that transaction that is responsible for keeping these saved-but-not-yet-committed changes, and even if that were not the case (this is cleared at several points during the transaction), the Infinispan cache should still be providing the transactional context with an MVCC view of the persisted-but-not-yet-committed documents.

I did notice something unusual during my debugging sessions. Even though I was not able to reproduce the test failure as reported in MODE-2202, if the test thread were paused for at least 60 seconds immediately after saving (but before committing or rolling back) the changes, the second session that should not see the changes actually **was able** to see the changes. After investigating further, my hypothesis is that the workspace cache was evicting the cached representation after 60 seconds of non-use (thanks to the workspace cache's eviction policy), so when the session attempted to verify that the changes could not be seen, it couldn't find the parent node's document in the workspace cache and had to load it from the Infinispan cache, and this document actually contained the changes that were persisted-but-not-yet-committed.

So something is definitely wrong: **the Infinispan cache and its MVCC feature should provide the transaction and only that transaction with a unique view containing all persisted-but-not-yet-committed documents; none of these persisted-but-yet-committed documents should be visible to those outside the transaction.** So if Infinispan **is** exposing the persisted-but-not-yet-committed documents (entries) to the other session, then one of two things is wrong:

1. Either there is a new bug in Infinispan 6.0 that is leading it to expose the MVCC changes to clients not affiliated with the transaction; or
1. Infinispan is convoluting the second session (which we think is in a separate transactional context) as being part of the other session's transactional context.

My belief is that we're actually seeing the second case: the `DummyTransactionManager` (which is what we're using in these tests) is associating the transaction **with the thread**. And because our tests do not suspend the transaction before using the second session, Infinispan lets both sessions see the MVCC changes affiliated with the transaction.

IMO, our unit tests are the source of the problem. We should not use a single thread for multiple sessions in separate transactional contexts. Instead, the tests should use a **separate thread** for each transactions.

This commit implements this fix to the unit tests, and after a little debugging I've confirmed that the tests work (even when running them 1000x in a row in the same VM) and that even after waiting the 60 seconds for the non-changed documents to be evicted from the workspace cache after changes are made using one of the sessions, the other session that should not see these changes in the documents it sees actually does not see the changes.

@hchiorean, please run these tests and uncomment the `main` method that runs the updated `shouldWorkWithUserDefinedTransactionsThatUseRollbackInSeparateThreads()` test 1000x. If it works for you, I would claim that what you saw was solely due to an incorrectly-written test.