ISPN-3665 SingleFileStore is not thread-safe for passivation

[pferraro]

No description provided.

[wburns]

This needs to be updated to be 2 instead of 3.

[danberindei]

The PR should also include the test.

[pferraro]

@danberindei I've rewritten the fix to address the underlying thread-safety issue, rather than working around it via forcing disk syncing. I've also included the test that reproduced the issue and validates the fix.

[mmarkus]

@pferraro test is failing on my machine https://gist.github.com/mmarkus/7352945

[pferraro]

I see the problem. Another thread can still modify the file entry after FileEntry.waitUnlocked() succeeds. So the waitUnlocked() AND file entry invalidation needs to happen while holding the fileList monitor.
Also, waitUnlocked() returns silently if interrupted, which is also wrong. The caller would assume that no thread holds a lock, but that's not necessarily true.
In general, the threading logic in this class is really sloppy - synchronizing on the file entry itself should be enough to prevent threads from writing to the entry. I recommend someone on your team revisit this at some point.

[wburns]

Looking closer this is a bit more complicated. I just created https://issues.jboss.org/browse/ISPN-3693 which is probably causing quite a bit of confusion as there is an issue with concurrent get and evicts with passivation when eviction is done manually. Basically it can cause the entry to be lost in that it is neither in the data container or in the store. Once that is fixed this should probably be revisited.

[pferraro]

I'm not denying that there aren't other issues with eviction, but nevertheless ISPN-3693 doesn't invalidate this fix.

[wburns]

@pferraro Oh don't get me wrong, I wasn't trying to say this fix was not needed. I was just saying that there are other issues other than just the SingleFileStore and the test you provided will not work properly unless ISPN-3693 is fixed as well.

And I totally agree that SingleFileStore should be revisited to be even more thread safe than it is atm 👍

[pferraro]

@mmarkus Is this test still failing for you? I've run in many times in a row with 100% success.

[danberindei]

@pferraro I have finally managed to reproduce a failure with a test that doesn't involve the ActivationInterceptor, and it looks like the fix here is not sufficient. What happens is that _load() releases the read lock on the FileEntry before unmarshalling, but keeps accessing its members keyLen, dataLen, and metadataLen. Another thread can write the entry to another location, add the FileEntry instance to the free list, and then reuse the FileEntry to store another cache entry. I'm closing this PR and I'll open another one with the new fix and the SingleFileStore-only test.

@wburns Do you still need Paul's test for PR #2203? If possible, I'd prefer a test that uses DummyInMemoryStore instead.

[pferraro]

@danberindei Thanks for looking into this. I suggest replacing the keyLen, dataLen, and metadataLen with a volatile reference to an immutable object. It would help improve the throughput of the store, I think, if concurrent readers didn't have to sync on the same monitor to guarantee thread-safe access to these values.

[wburns]

@danberindei Created https://issues.jboss.org/browse/ISPN-3699 to cover the tests for the other changes since this PR is closed now.