JCR-4608 Upgrade to commons-collections4 #128
Conversation
|
can you try to rebase this based on what's currently in trunk? |
| public void remove() { | ||
| throw new UnsupportedOperationException("remove"); | ||
| } | ||
|
|
There was a problem hiding this comment.
I stumbled across this as well; the original code apparently tried to protect itself against modifcations through other methods. Can we really eliminate this?
There was a problem hiding this comment.
I've added a test for concurrent modification. Was passing when I accessed the original modCount field via reflection but doesn't work when I overlay the field. Must be a stupid mistake but I can't see it 🤔
rkovarik
force-pushed
the
JCR-4785
branch
2 times, most recently
from
2742c0f
to
21de8c9
Compare
| @Test(expected = ConcurrentModificationException.class) | ||
| public void concurrentModification() throws Throwable { | ||
| ChildNodeEntriesImpl entries = new ChildNodeEntriesImpl(null, null, null); | ||
| Collection<Future<?>> futures = new ArrayList<>(); |
There was a problem hiding this comment.
@rkovarik the test failing looks like a timing issue. If you pre-populate the child nodes entries it has a greater chance to actually run into a ConcurrentModificationException.
I.e. adding 100 child nodes did the trick for me.
for (int k = 0; k < 100; k++) {
entries.add(mock(NodeEntry.class));
}
There was a problem hiding this comment.
Right, thx @mduerig. I get only up to 1000 successful runs so I will remove the test, the code looks fine.
There was a problem hiding this comment.
I'd still like to see a test (that we can make fail by removing the modified checks in the old code) before proceeding.
There was a problem hiding this comment.
OK, pushed it back, one timing failure in ~10 000 runs. fab2723
There was a problem hiding this comment.
The test is non deterministic so I'd expect it to fail from time to time. I don't see a way to implement a deterministic test here unless we make LinkedEntries package private and unit test that class directly.
There was a problem hiding this comment.
I already moved it into a separate class (see https://issues.apache.org/jira/browse/JCR-4854)
This reverts commit 7d36c33.
|
@rkovarik , @mduerig - please see https://issues.apache.org/jira/browse/JCR-4862 (which is the last non-trivial step in removing the old dependency) |
|
(OBE) |
https://issues.apache.org/jira/browse/JCR-4608
commons-collections 3.2.2 suffers from https://advisory.checkmarx.net/advisory/vulnerability/Cx78f40514-81ff/
Although Jackrabbit itself does not seem to use SetUniqueList, it still exists on the classpath.